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 custom url #1607

Merged
Merged

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Sep 12, 2022

Subject

I am targeting this branch, because this is a bug fix.

Closes #502

Changelog

### Fixed
- Fix `customUrl` implementation.

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@VincentLanglet
Copy link
Member

Does a test should be added ?

You generally add a test which is failing yhe ci then yiu fix the bug.

Here you changed the code (failing the ci) then you fixed the test. Does it mean test was wrong ?

@jordisala1991
Copy link
Member Author

code changed behavior, that's why test was wrong.

I will try to see if a test can be added, but not sure it is easy.

@jordisala1991 jordisala1991 force-pushed the hotfix/fix-custom-url branch 3 times, most recently from 435dba9 to a0be0fc Compare September 13, 2022 16:35
@jordisala1991
Copy link
Member Author

jordisala1991 commented Sep 13, 2022

Test added, both for pages and snapshots, @VincentLanglet .

The thing is: the URL is not editable by the user. The user can only add pages being child of other pages, with a hierarchy like:

  • Home (url: '/')
  • Inner: (url: '/test') (because on slug we put "test")
  • Another... (url: '/test/test2') (because child of inner and we put "test2" on slug)

The other field you can change is the customURL, that way the url will be whatever you place on that field, like: '/test1/test2/test3', without the need of the full hierarchy. Also this field is only usable on CMS pages, NOT hybrid or dynamic (those pages fixes the url through symfony routing).

IMO it makes sense to keep using everywhere the url field. there was another PR proposing to do some match between url and custom url, but that complicates things IMO.

The only thing that was taken into account is the relativePath of the SITE

@VincentLanglet VincentLanglet merged commit 01f5a86 into sonata-project:4.x Sep 13, 2022
@jordisala1991 jordisala1991 deleted the hotfix/fix-custom-url branch September 13, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants