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

Fix slug generation if the getRegenerateSlugOnUpdate method return false #472

Merged
merged 7 commits into from
Dec 28, 2019

Conversation

hermann8u
Copy link
Contributor

This PR aims to fix the bug described in the issue #471

If the getRegenerateSlugOnUpdate method return false, the slug will be generate if its value is null.

This behavior seems legit because you can set the slug manually and it won't be regenerated. However if you set manually the slug value to null, it will be regenerated (the setter doesn't allow this anyway).

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Dec 25, 2019

Wow, thank you for finding time! 🎄

Could you also add a test case to show, what went wrong? So we don't have this bug in the future due to unrelated changes.

@hermann8u
Copy link
Contributor Author

hermann8u commented Dec 28, 2019

Any idea how to ignore this blocking "CS" check?

The getRegenerateSlugOnUpdate method is private on the trait but is considered as unused in the CS check when I override it in the implementing entity.

EDIT

Ok, I just add the class to the skip section for this rule of the CS config

* @ORM\Column(type="datetime")
* @var DateTimeInterface
*/
private $date;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this property used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not but I keep it from the SluggableEntity I copied. The goal of this property is to have an other property which is not concerned by the slug generation. For the moment, this property is useless, so we have 3 choices:

  • Keep it for future test case
  • Remove it because there is no test based on it
  • Add test concerning it like in Knp\DoctrineBehaviors\Tests\ORM\SluggableTest class

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove it in both, because it doesn't add any value

Copy link
Contributor Author

@hermann8u hermann8u Dec 28, 2019

Choose a reason for hiding this comment

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

The value of the date property in Knp\DoctrineBehaviors\Tests\Fixtures\Entity\SluggableEntity is to test that the slug is not updated when we change an unrelated value.
But in Knp\DoctrineBehaviors\Tests\Fixtures\Entity\SluggableWithoutRegenerateEntity, there is no test with this date property so I agree to delete it.

@TomasVotruba
Copy link
Contributor

Ok, I just add the class to the skip section for this rule of the CS config

👍

Thanks for adding test case here.

@TomasVotruba TomasVotruba merged commit 6209178 into KnpLabs:master Dec 28, 2019
@TomasVotruba
Copy link
Contributor

Thank you 👍

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