dereuromark / cakephp-tools

A CakePHP Tools plugin containing lots of useful helpers, behaviors, components, shells, ...
http://www.dereuromark.de
MIT License
336 stars 142 forks source link

Slug edited when label field is not present in the POST request #293

Closed frandss closed 7 months ago

frandss commented 7 months ago

I think this behavior is not expected.

With this SluggedBehavior configuration:

$this->addBehavior('Tools.Slugged', [ 'overwrite' => true ]);

If label field is not present in the entity along the save process, behavior sets the slug field to a empty string, changing the original slug.

In the behavior doc https://github.com/dereuromark/cakephp-tools/blob/master/docs/Behavior/Slugged.md we can read in overwrite option specification: "true: if the label field values change, regenerate the slug (use if you are the slug is just window-dressing)".

If the label field is not present in the edit request, label field value does not change, then, slug should not change.

With this changes, if any label field is present, the system don't change slug.

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 80.01%. Comparing base (67063a9) to head (6c3e607).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #293 +/- ## ============================================ + Coverage 80.00% 80.01% +0.01% - Complexity 1949 1951 +2 ============================================ Files 79 79 Lines 5466 5470 +4 ============================================ + Hits 4373 4377 +4 Misses 1093 1093 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dereuromark commented 7 months ago

Can we have atest case for it that Shows it was broken before and is now fixed?

dereuromark commented 7 months ago

This test seems to show that the current behavior is not broken: https://github.com/dereuromark/cakephp-tools/commit/48bfcb9889ca6cf9abac6e1a4e291e41a712baf0

Can you elaborate whats different in your case?

dereuromark commented 7 months ago

Oh I think I see what you mean: The entity is not fully hydrated and as such the save process then changes the slug.

For overwrite to work it is currently indeed necessary to have always full entity hydrated. The other option could be to check isDirty() on all slug fields, and if all are false to ignore the update process.

dereuromark commented 7 months ago

Added a test case in https://github.com/dereuromark/cakephp-tools/commit/0da38f2080b1faa5b748318f02a410021ead8aac Thank you!

dereuromark commented 7 months ago

If you can confirm it is all fixed, I can release a patch version

frandss commented 7 months ago

Great job! It works fine. I've started to have this problem in the password change method in CakeDC/Users plugin. This method only save password field on user entity, and slug change to empty string. This method and all other tests already work fine.

Thanks for improvements and update.

dereuromark commented 7 months ago

https://github.com/dereuromark/cakephp-tools/releases/tag/3.6.2