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

SS5: move updateRelativeLink hook after concatination #2770

Merged
merged 5 commits into from
Aug 31, 2022

Conversation

xini
Copy link
Contributor

@xini xini commented Aug 26, 2022

This is to make the relative actually updatable.

This picks up #2677 again for SS5.

@xini xini changed the title move updateRelativeLink hook after concatination SS5: move updateRelativeLink hook after concatination Aug 26, 2022
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

There are CI failures to address.

code/Model/SiteTree.php Show resolved Hide resolved
@GuySartorelli
Copy link
Member

What, ultimately, is the intention here?

@xini
Copy link
Contributor Author

xini commented Aug 26, 2022

The intention is to be able to actually update the relative link. If the link is concatenated after the hook, you can't update the final link.

@GuySartorelli
Copy link
Member

We'll need some words about this in the changelog but the PR that adds the 5.0.0 changelog file hasn't been merged yet. I won't let that hold up this change - instead I'll make a note for myself to add that to the changelog once it exists.

@GuySartorelli
Copy link
Member

SiteTreeExtension includes a method signature for updateRelativeLink(&$base, &$action) - can you please update both the method signature and the PHPdoc in that extension?

Looks like in core there's no other instance of that method being defined - there might be in some of the supported modules but we haven't started looking at those yet for CMS 5

@xini
Copy link
Contributor Author

xini commented Aug 29, 2022

Done. Sorry I missed that.

code/Model/SiteTreeExtension.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member

Changelog PR: silverstripe/developer-docs#75

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.

2 participants