Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hybrid support for BelongsToMany relationship #2688

Merged

Conversation

hans-thomas
Copy link
Contributor

@hans-thomas hans-thomas commented Nov 27, 2023

Hi, I noticed the lines that we removed in #2664 (comment) PR, were part of handling the hybrid relationship that was not covered by tests.
This PR is continuing #2276. Thanks to @juniohyago.

  • Updating eloquent model instance with latest relation data

hans-thomas and others added 3 commits November 27, 2023 14:27
Co-Authored-By: Junio Hyago <35033754+juniohyago@users.noreply.github.com>
Co-Authored-By: Junio Hyago <35033754+juniohyago@users.noreply.github.com>
@hans-thomas hans-thomas marked this pull request as ready for review November 27, 2023 11:20
@hans-thomas hans-thomas marked this pull request as draft November 28, 2023 12:19
@hans-thomas
Copy link
Contributor Author

The relation data of the current eloquent model instance doesn't update. It has to be fixed to prevent users from fetching the latest relationship data from DB each time or it's possible to make users confused.

@hans-thomas hans-thomas marked this pull request as ready for review November 29, 2023 11:37
@@ -51,5 +57,12 @@ public static function executeSchema(): void
$table->string('name');
$table->timestamps();
});
if (! $schema->hasTable('skill_sql_user')) {
$schema->create('skill_sql_user', function (Blueprint $table) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to have a pivot table here? If the relations are already stored in the MongoDB document, it should not be duplicated in an SQL table.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Check if it is a relation with an original model.
if (! is_subclass_of($related, MongoDBModel::class)) {
return parent::belongsToMany(
$related,
$collection,
$foreignPivotKey,
$relatedPivotKey,
$parentKey,
$relatedKey,
$relation,
);
}

The sql models don't reach our BelongsToMany class. So, we can't control this behavior.
As I know, the sql models, store their relations data in a pivot tabel and mongo models, store theirs in a pivot column.

@hans-thomas
Copy link
Contributor Author

Hi @GromNaN, Could you please merge these two PRs?
I know you're super busy but I will be gone from next week for at least 2 months (it can be increased to one and a half years) and I don't have access to the Internet in that period.
Is there any chance to release the v4.1?

@GromNaN
Copy link
Member

GromNaN commented Dec 11, 2023

It seems feasible today.

@hans-thomas
Copy link
Contributor Author

It would be awesome😁🔥
before releasing it, I want to open one more PR for BelongsToMany relation. I noticed some potential bugs when we set custom keys.

@GromNaN GromNaN merged commit 2adbf87 into mongodb:4.1 Dec 11, 2023
15 checks passed
@GromNaN
Copy link
Member

GromNaN commented Dec 11, 2023

Thank you @hans-thomas. I'm merging the other one and wait for your new PR.

@hans-thomas hans-thomas deleted the add-hybrid-support-to-belongs-to-many branch December 11, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants