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

Make blog post titles unique #1062

Merged
merged 3 commits into from Jan 15, 2020
Merged

Conversation

@javiereguiluz
Copy link
Member

javiereguiluz commented Jan 7, 2020

I'm trying to fix #1045.

We should make the slug unique because it's what we use to look for blog posts in the database. However, I can't make this work. If I define UniqueEntity on title, it works ... but if I define it on slug, it doesn't work (I can create any number of posts with the same slug). Anybody sees the error? Thanks!

use Symfony\Component\Validator\Constraints as Assert;

/**
* @ORM\Entity(repositoryClass="App\Repository\PostRepository")
* @ORM\Table(name="symfony_demo_post")
* @UniqueEntity(fields={"slug"}, errorPath="title")

This comment has been minimized.

Copy link
@yceruto

yceruto Jan 7, 2020

Member

Yes, the slug value is generated after $form->isValid() (when the validation process is fired) in Admin\BlogController.php.

To make it work properly we can move this statement $post->setSlug(Slugger::slugify($post->getTitle())) before $form->isValid() and after $form->handleRequest($request). What do you think?

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jan 7, 2020

Author Member

Yes! That must be the error.

However, if I move that line ... I see this error:

Argument 1 passed to Symfony\Component\String\Slugger\AsciiSlugger::slug() must be of the type string, null given.

If I change it like this:

        $form->handleRequest($request);

        if ($form->isSubmitted()) {
            $post->setSlug($slugger->slug($post->getTitle())->lower());
        }

        if ($form->isSubmitted() && $form->isValid()) {
            // ...
        }

Then I can again create multiple blog posts with the same slug, so the validation is not working.

This comment has been minimized.

Copy link
@voronkovich

This comment has been minimized.

Copy link
@yceruto

yceruto Jan 7, 2020

Member

Sorry for the confusion, @voronkovich is right, is the $form->handleRequest() who fires the validation process, so we're at the beginning again :)

This comment has been minimized.

Copy link
@voronkovich

voronkovich Jan 7, 2020

Contributor

@javiereguiluz, @yceruto I would suggest to use a form event listener:

->addEventListener(FormEvents::SUBMIT, function (FormEvent $event) {
    $post = $event->getData();
    $post->setSlug($this->slugger->slug($post->getTitle())->lower());
})

This also will decrease code duplicity, because we won't need to generate slug in both new and edit controllers.

This comment has been minimized.

Copy link
@yceruto

yceruto Jan 7, 2020

Member

What about a custom validation by using the repositoryMethod option of the @UniqueEntity() annotation?

There we might have the chance to build the slug before checking that it's unique.

This comment has been minimized.

Copy link
@yceruto

yceruto Jan 7, 2020

Member

This line $post->setSlug($this->slugger->slug($post->getTitle())->lower()); is unsafe, you'll need to check before that that the title value is not null. But yes, that's another option.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jan 8, 2020

Author Member

I liked both solutions a lot. I think they are both equally correct. However, I decided to try the form event solution proposed by @voronkovich because it felt more closely related to Symfony. A repository is a Doctrine thing, not a strictly Symfony thing and that's why I think we should use form events in this case. But I'm open to hear other opinions and change this. Thanks!

@voronkovich

This comment has been minimized.

Copy link
Contributor

voronkovich commented Jan 8, 2020

@javiereguiluz, should we add a functional test for this feature?

Javier Eguiluz Javier Eguiluz
javiereguiluz added a commit that referenced this pull request Jan 15, 2020
This PR was merged into the master branch.

Discussion
----------

Make blog post titles unique

I'm trying to fix #1045.

We should make the `slug` unique because it's what we use to look for blog posts in the database. However, I can't make this work. If I define `UniqueEntity` on `title`, it works ... but if I define it on `slug`, it doesn't work (I can create any number of posts with the same slug). Anybody sees the error? Thanks!

Commits
-------

178e2b1 Added a functional test
7486dc8 Added a form event to set the slug
a4a316a Make blog post titles unique
@javiereguiluz javiereguiluz merged commit 178e2b1 into symfony:master Jan 15, 2020
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.