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

Check all fields for new entities. #17668

Closed
wants to merge 1 commit into from

Conversation

nicosp
Copy link
Contributor

@nicosp nicosp commented Apr 15, 2024

Not sure why but it restored behavior to 4.x. Without the change it skips the check for new entities.

For some reason: $entity->extract($this->_fields, true) returns [] for some new entities that it wasnt before. This was found during phpunit when testing our 5.x upgrade branch.

@nicosp nicosp marked this pull request as draft April 15, 2024 13:12
@dereuromark
Copy link
Member

Could this have been a merge error maybe? with resolving a conflict?

@dereuromark dereuromark added this to the 5.0.8 milestone Apr 15, 2024
@othercorey
Copy link
Member

What restored this behavior? Can you explain more?

@dereuromark
Copy link
Member

I also dont see how this was changed as "revert" as you claim.
https://github.com/cakephp/cakephp/blob/5.0.0-beta1/src/ORM/Rule/IsUnique.php
Even the beta didn't have any such change yet.

@nicosp
Copy link
Contributor Author

nicosp commented Apr 16, 2024

I am not sure why but without this change our unique rules do not work for some cases. The fix is most definitely wrong and its an attempt to figure it out. Sorry for the mix up. I updated the comment above.

@nicosp
Copy link
Contributor Author

nicosp commented Apr 16, 2024

Turns out the issue was that we had rules with the same name. The last rule with the same name "won" and effectively cancelled the previous ones.

@nicosp nicosp closed this Apr 16, 2024
@nicosp nicosp deleted the fix-isunique-for-new branch April 16, 2024 07:23
@ADmad ADmad added invalid and removed bug labels Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants