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

[TASK] Allow urls without .html for CMS. #936

Merged
merged 3 commits into from
Mar 29, 2019

Conversation

lewisvoncken
Copy link
Contributor

Description

Related Issue

Closes #ISSUENUM.

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Proposed Labels for Change Type/Package

Checklist:

  • I have read the CONTRIBUTING document.
  • I have linked an issue to this PR.
  • I have indicated the change type and relevant package(s).
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All CI checks are green (linting, build/deploy, etc).
  • At least one core contributor has approved this PR.

@vercel
Copy link

vercel bot commented Feb 22, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@coveralls
Copy link

coveralls commented Feb 22, 2019

Coverage Status

Coverage remained the same at 76.512% when pulling a0f84e0 on lewisvoncken:patch-1 into bb0f376 on magento-research:develop.

@awilcoxa
Copy link

@lewisvoncken can you add additional detail here and include an issue so that the team can review?

@awilcoxa awilcoxa added the hold On hold until another condition is fulfilled. label Feb 27, 2019
@awilcoxa
Copy link

placing on hold until detail is added

@lewisvoncken
Copy link
Contributor Author

@awilcoxa
There is no issue related to this Pull Request but here is a description why this should be merged:

In Magento it is possible to use a suffix in the url which is a configuration option

By default this option is set to .html but only for Catalog Urls

Because of this the CMS page routing won't work correctly because these rewrites don't have .html

To make CMS page routing work correctly it should allow urls without .html Besides that the functionality won't change and the support of a different suffix is also added through this change.

@supernova-at supernova-at changed the base branch from release/2.0 to develop March 6, 2019 17:02
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

Thank you! Excellent addition, just needs the outer conditional to be reordered.

@@ -190,7 +190,7 @@ cmsPageResult:
urlKey:
when:
- matches: request.url.pathname
pattern: '^/(?:(.*)\.html)?'
pattern: '^/(?:(.*))?'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the right idea (until we have the store config introspection requested in magento/graphql-ce#535) but it needs to go further down in the stack. Since CMS pages have no suffixes, they now live in the same namespace as some of our hardcoded routes, like checkout. Those hardcoded routes need to resolve first--otherwise, an administrator could accidentally break their store by writing an SEO Friendly URL that conflicts with a builtin PWA route, AND, UPWARD would place the urlResolver call unnecessarily for every single request.

@zetlen zetlen assigned zetlen and unassigned zetlen Mar 26, 2019
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.

5 participants