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

Added ORDER BY to delete operation + update composer.json + fix tests #7

Merged
merged 21 commits into from
Apr 10, 2022

Conversation

ildyria
Copy link
Member

@ildyria ildyria commented Apr 10, 2022

Copy of #6 to v6 branch (to align with upstream repo with v6).

Partially fixes LycheeOrg/Lychee#1251

As MySQL does not comly to ANSI SQL and does not support statement consistency, but only simple row consistency, the order of rows is important when a complete sub-tree is deleted.

@ildyria ildyria requested a review from kamil4 April 10, 2022 12:45
@nagmat84
Copy link
Collaborator

I haven't checked the details, but did you only migrate #6 on top of the v6 branch? Please keep in mind that we already had to merge a couple of other bug fixes into our copy of the v5 branch, inter alia

  • another fix for deletion some month ago
  • a fix which uses a proper interface to check whether a model is a nested set node
  • some fixes with respect to exception handling

This are only the fixes a do remember. Maybe there are more.

So basically, our v5 branch and the upstream v5 were diverted (not in the git sense, but in the sense that we had to fix a lot which never made it upstream). Have you checked that all these bug fixes are also in "our" v6?

@ildyria
Copy link
Member Author

ildyria commented Apr 10, 2022

I took their v6, merged into your branch (which is based on our diverged v5) and created a pull request to a copy of their v6.

If you like git log --one-line
2022-04-10_1920x1080_15:31:55

So unless I'm wrong, this PR to v6 basically contains all the changes you made to the v5 + #6 + passing tests :)

@nagmat84
Copy link
Collaborator

Understood. So I still need to find out what the original difference between upstream v5 and v6 is. I am always a little bit conservative with major upgrades.

@ildyria
Copy link
Member Author

ildyria commented Apr 10, 2022

Understood. So I still need to find out what the original difference between upstream v5 and v6 is. I am always a little bit conservative with major upgrades.

It is just a bump of the Laravel version, code wise there is pretty much 0 change... I seriously do not know why they decided to make it v6. IMHO that was not needed.
The only reason why I would be in favor to align with them is to make it easier to integrate some of the potential fixes they may have in the future.

You can clearly see that their v6 does not add anything, especially given that the merge commit is quite empty:
f47602e

As for ec8e1d7
It is a fix that was applied on the v5 after the v5-v6 split on their repo.
I took the initiative to merge it here.

Copy link
Collaborator

@nagmat84 nagmat84 left a comment

Choose a reason for hiding this comment

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

Some minor changes

ildyria and others added 2 commits April 10, 2022 16:34
Co-authored-by: Matthias Nagel <[email protected]>
Co-authored-by: Matthias Nagel <[email protected]>
Copy link
Collaborator

@nagmat84 nagmat84 left a comment

Choose a reason for hiding this comment

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

LGTM.

@ildyria ildyria merged commit f48c1ad into v6 Apr 10, 2022
@ildyria ildyria deleted the fix_order_of_deletion_v6 branch April 10, 2022 14:49
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.

2 participants