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

Add ability to override routing with record_route #1484

Merged
merged 5 commits into from
Jun 17, 2020

Conversation

I-Valchev
Copy link
Member

Fixes #1483
Fixes #1470

One outstanding issue:

termsandconditions:
  path: /terms-and-conditions
  defaults:
    _controller: Bolt\Controller\Frontend\DetailController::record
    contentTypeSlug: 'terms-and-conditions'
    slugOrId: 31

The default slugOrId is set here.
Trying to get the |link on the /terms-and-conditions page breaks the url, because the ContentExtension::getLink always overrides the slugOrId param, hence the link becomes:
/terms-and-conditions?slugOrId=someUnexpectedSlug

where someUnexpectedSlug is the slug from the content.

I'm not sure how to fix this elegantly? @bobdenotter

@I-Valchev I-Valchev changed the title [WIP] record_route option. clean up canonical. [WIP] Add ability to override routing with record_route Jun 15, 2020
@bobdenotter
Copy link
Member

I see the problem!

First, it works for the generated canonical just fine, it seems:

Screenshot_2020-06-16_at_08_02_11

But, it doesn't work correctly when using {{ record|link }}:

Screenshot 2020-06-16 at 08 11 07

This is because it's configured with some defaults, and then you generate a route for something that has different values for these specific things. This is undesirable, but it's also the correct behaviour, I think.

If i'd set those values as defaults (and make sure there's an entry with slug terms-and-conditions), it doesn't add them:

termsandconditions:
  path: /terms-and-conditions
  defaults:
    _controller: Bolt\Controller\Frontend\DetailController::record
    contentTypeSlug: 'entry'
    slugOrId: terms-and-conditions

Screenshot 2020-06-16 at 08 15 58

I think this is not something we can fix, but it does have a workaround..

What's more annoying is the following:

content_seo:
   path: /{slugOrId}
   methods: [GET|POST]
   defaults:
     _controller: Bolt\Controller\Frontend\DetailController::record

That'll make any /lorem-ipsum work, but if i add record_route: content_seo, the result of {{ record|link }} is /.. There it doesn't substitute the slugOrId, while it really should do that.

@I-Valchev
Copy link
Member Author

I see now!
Yeah that makes sense, if it's configured in routes.yaml with the slug and not ID, they are the same, so no query params.

I am not sure I could replicate the content_seo example. You say that any /lorel-ipsum should work, but for me when I try anything site.wip/lorem-ipsum it says content not found. Which I think makes sense given the controller:

        if (is_numeric($slugOrId)) { // THIS IS FALSE
            $record = $this->contentRepository->findOneBy(['id' => (int) $slugOrId]);
        } else {
           // $contentTypeSlug is null here?
            $contentType = ContentType::factory($contentTypeSlug, $this->config->get('contenttypes'));
            $record = $this->contentRepository->findOneBySlug($slugOrId, $contentType);
        }

        return $this->renderSingle($record, $requirePublished);

I was able to get it working if I change ContentRepository::findOneBySlug on line 146 from if ($contentType) { to
if ($contentType && $contentType->get('slug')) {

And then the |link is /aut-error-cupiditate-magnam?contentTypeSlug=blog-post

@bobdenotter
Copy link
Member

And then the |link is /aut-error-cupiditate-magnam?contentTypeSlug=blog-post

Still, that's better than it was before. If you fix the tests (seems pretty straightforward in this case), let's merge and move to the next item on the list for now! :-)

@I-Valchev I-Valchev changed the title [WIP] Add ability to override routing with record_route Add ability to override routing with record_route Jun 16, 2020
@I-Valchev I-Valchev changed the title Add ability to override routing with record_route [WIP] Add ability to override routing with record_route Jun 17, 2020
@I-Valchev I-Valchev changed the title [WIP] Add ability to override routing with record_route Add ability to override routing with record_route Jun 17, 2020
Copy link
Member

@bobdenotter bobdenotter left a comment

Choose a reason for hiding this comment

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

Neat! 🥃

@bobdenotter bobdenotter merged commit 1c60236 into master Jun 17, 2020
@bobdenotter bobdenotter deleted the feature/override-record-routes branch June 17, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overriding routes.yaml breaks |link unexpectedly Specify route name in ContentType config
2 participants