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

EZP-31110: Fixed URLAlias utilizing tree_root.location_id and place at site root options #3065

Closed
wants to merge 4 commits into from

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Sep 16, 2020

Question Answer
JIRA issue EZP-31110
Bug yes
New feature no
Target version 7.5
BC breaks no
Tests pass yes
Doc needed no

Using place at site root option in case of using tree_root.location_id would never match any URLAlias because of $pathPrefix being included into search when tree_root.location_id is used. Therefore, additional try has been added to account for URLAlias which might be placed at the site root.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@barw4 barw4 self-assigned this Sep 16, 2020
@barw4 barw4 changed the title EZP-31110: Fix URLAlias utilizing tree_root.location_id option EZP-31110: Fixed URLAlias utilizing tree_root.location_id option Sep 16, 2020
@barw4 barw4 changed the title EZP-31110: Fixed URLAlias utilizing tree_root.location_id option [WIP] EZP-31110: Fixed URLAlias utilizing tree_root.location_id option Sep 16, 2020
@andrerom
Copy link
Contributor

Is this perhaps a misunderstanding of the feature?

Afaik "place at site root" is badly named, it really is about "place at content root" as repo for the most part knows nothing about what a site is.

We should maybe check with what legacy did here, and if we want a site root feature then we either need to let user pick site it should be on to deduct path based on root location of that site.

Or in the case of virtual URL's like what you are proposing here, then we need to make it clear it will be available in all sites, even if content might not be available.

@barw4
Copy link
Member Author

barw4 commented Sep 17, 2020

Afaik "place at site root" is badly named, it really is about "place at content root" as repo for the most part knows nothing about what a site is.

Isn't it how it behaves? Site root (cuts URL when creating custom URL) = content root if option tree_root.location_id is configured in a given SA. It's just the naming after option from the Custom URLAlias's form. This PR is not about URLAlias being SiteAccess aware. The check for what content root is set for a given SA was already done like that before, I haven't changed this behavior.

Or in the case of virtual URL's like what you are proposing here, then we need to make it clear it will be available in all sites, even if content might not be available.

Why would it be available? If the SA has such Content available due to .yml config param mentioned above it should match. If we have a Custom URL: /foo/bar with used "place at site root" option checked one would expect it will match the following URL:
[SA]/foo/bar where tree/root is the root location of SA we are accessing and the Content is located at tree/root/foo/bar.

@barw4 barw4 changed the title [WIP] EZP-31110: Fixed URLAlias utilizing tree_root.location_id option EZP-31110: Fixed URLAlias utilizing tree_root.location_id option Sep 17, 2020
@barw4 barw4 changed the title EZP-31110: Fixed URLAlias utilizing tree_root.location_id option EZP-31110: Fixed URLAlias utilizing tree_root.location_id and place at site root options Sep 17, 2020
@andrerom
Copy link
Contributor

TBH a bit unsure, but I can't remember legacy having to do two lookups to UrlAlias like this so might need a look by Engineering / PM on how this feature is intended to behave. /cc @SylvainGuittard

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

As André mentioned, it requires PM input.
Either SylvainGuittard or bdunogier provide a specification for this feature please.

@alongosz alongosz dismissed their stale review September 28, 2020 10:31

Retracting, we'll investigate the needs to decide how it should behave

@barw4
Copy link
Member Author

barw4 commented Oct 13, 2020

Closed in favor of ezsystems/ezplatform-admin-ui#1583

@barw4 barw4 closed this Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants