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

Allow multiple entities with closure strategy #2653

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

JDruery
Copy link

@JDruery JDruery commented Jul 24, 2023

Fixes #2652. I added one test that does not pass and then modified the Closure.php strategy file to handle this case (persisting two types of entities, both with closure trees, and then flushing).

@phansys phansys added Tree Bug A confirmed bug in Extensions that needs fixing. labels Jul 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2023

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.37%. Comparing base (85f7831) to head (fad0036).
Report is 73 commits behind head on main.

Current head fad0036 differs from pull request most recent head e6c6de6

Please upload reports for the commit e6c6de6 to get more accurate results.

Files Patch % Lines
src/Tree/Strategy/ORM/Closure.php 85.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2653      +/-   ##
==========================================
- Coverage   79.67%   79.37%   -0.30%     
==========================================
  Files         161      162       +1     
  Lines        8453     8471      +18     
==========================================
- Hits         6735     6724      -11     
- Misses       1718     1747      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.docker/php/Dockerfile Outdated Show resolved Hide resolved
src/Tree/Strategy/ORM/Closure.php Outdated Show resolved Hide resolved
src/Tree/Strategy/ORM/Closure.php Outdated Show resolved Hide resolved
tests/Gedmo/Tree/Issue/Issue2652Test.php Outdated Show resolved Hide resolved
tests/Gedmo/Tree/Issue/Issue2652Test.php Outdated Show resolved Hide resolved
tests/Gedmo/Tree/Fixture/Issue2652/Category2.php Outdated Show resolved Hide resolved
src/Tree/Strategy/ORM/Closure.php Show resolved Hide resolved
src/Tree/Strategy/ORM/Closure.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Given the tied implementation we have around spl_object_id() for array keys, I think we could extract that logic to a private method instead of manually building these keys each time.
BTW, please, don't forget to add a changelog note.

@JDruery
Copy link
Author

JDruery commented Jul 31, 2023

Given the tied implementation we have around spl_object_id() for array keys, I think we could extract that logic to a private method instead of manually building these keys each time. BTW, please, don't forget to add a changelog note.

I went ahead and added a changelog note. Let me know if I did it correctly.

If you can provide specific instructions on what other refactoring you'd like to see, I could try to add that to this PR, but I wonder if that would be better to put in a separate update.

JDruery added a commit to JDruery/DoctrineExtensions that referenced this pull request Jul 31, 2023
@phansys
Copy link
Collaborator

phansys commented Aug 1, 2023

I went ahead and added a changelog note. Let me know if I did it correctly.

It is correct, thank you.

If you can provide specific instructions on what other refactoring you'd like to see, I could try to add that to this PR, but I wonder if that would be better to put in a separate update.

I agree, let's make this change in a separate PR.

@JDruery
Copy link
Author

JDruery commented Aug 14, 2023

I'm still having issues, even after this fix. I've added one more commit to simplify the code further and ensure that closures are never added until the node has been persisted. I think the issue is solved now, but let me know if anything else needs to be done on my end.

CHANGELOG.md Outdated Show resolved Hide resolved
src/Tree/Strategy/ORM/Closure.php Outdated Show resolved Hide resolved
src/Tree/Strategy/ORM/Closure.php Outdated Show resolved Hide resolved
@JDruery
Copy link
Author

JDruery commented Sep 8, 2023

I'm not sure if the rebase needs to be done by me, or someone else will do that. If it's me, I will need some instructions as I've never done this before.

@phansys
Copy link
Collaborator

phansys commented Sep 8, 2023

I'm not sure if the rebase needs to be done by me, or someone else will do that. If it's me, I will need some instructions as I've never done this before.

You should execute this command:

git pull --rebase upstream main

(where upstream is the name of the git remote pointing to this repo)

Then, you must manually resolve the conflicts presented by git, add the modified files to the staging area (git add) and continue with the rebase (git rebase --continue).
When the rebase is done, you must force push your changes:

git push origin main -f

JDruery added a commit to JDruery/DoctrineExtensions that referenced this pull request Sep 12, 2023
@JDruery
Copy link
Author

JDruery commented Sep 12, 2023

I'm not sure if the rebase needs to be done by me, or someone else will do that. If it's me, I will need some instructions as I've never done this before.

You should execute this command:

git pull --rebase upstream main

(where upstream is the name of the git remote pointing to this repo)

Then, you must manually resolve the conflicts presented by git, add the modified files to the staging area (git add) and continue with the rebase (git rebase --continue). When the rebase is done, you must force push your changes:

git push origin main -f

I've completed the rebase. Hopefully everything is correct, but let me know if anything else needs to be done related to that. Thanks.

@JDruery JDruery requested a review from phansys November 10, 2023 02:38
Copy link

github-actions bot commented May 8, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added Stale and removed Stale labels May 8, 2024
@franmomu franmomu added the Still Relevant Mark PRs that might've been auto-closed that are still relevant. label Jun 9, 2024
@JDruery
Copy link
Author

JDruery commented Aug 23, 2024

Any movement on this pull request? Anything still needed to merge this?

@phansys
Copy link
Collaborator

phansys commented Aug 25, 2024

Any movement on this pull request? Anything still needed to merge this?

IMO, this only requires a rebase currently.

@JDruery
Copy link
Author

JDruery commented Aug 26, 2024

Any movement on this pull request? Anything still needed to merge this?

IMO, this only requires a rebase currently.

Thanks for reviewing that, I went ahead and did a rebase.

composer.json Outdated
@@ -40,7 +40,7 @@
"wiki": "https://github.com/Atlantic18/DoctrineExtensions/tree/main/doc"
},
"require": {
"php": "^7.4 || ^8.0",
"php": "^7.2 || ^8.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a regression.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing that out, I mistakenly resolved the conflict in the wrong direction. I've updated the version to 7.4

composer.json Outdated
"symfony/phpunit-bridge": "^6.0 || ^7.0",
"symfony/uid": "^5.4 || ^6.0 || ^7.0",
"symfony/yaml": "^5.4 || ^6.0 || ^7.0"
"doctrine/orm": "^2.14.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, check the whole file. Many of these changes seem like a regression.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your patience, this is my first time contributing to a large project. I've gone through the whole file and updated all packages to match the current versions.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking the time to iterate and fix, JDruery!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A confirmed bug in Extensions that needs fixing. Still Relevant Mark PRs that might've been auto-closed that are still relevant. Tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tree] Inserts null ids into closure table when persisting more than one type of entity
5 participants