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 Do not update LeftAndMain link with Stage param #173

Conversation

maxime-rainville
Copy link
Contributor

Add a condition so we don't try to add Stage URL param to LeftAndMain links.

Fixes silverstripe/silverstripe-admin#607

@dhensby
Copy link
Contributor

dhensby commented Sep 1, 2018

This is a bit gross as we're mixing knowledge about modules in a way we shouldn't.

It would be much nicer to have a way for LeftAndMain to disable this link re-writing; can't we disable/remove the extension for the LeftAndMain controller?

@lukereative
Copy link

lukereative commented Sep 3, 2018

Since this extension is applied to RequestHandler I don't think we'll be able to remove it specifically on LeftAndMain according to this note on remove_extension: Note: This will not remove extensions from parent classes, and must be called directly on the class assigned the extension.

@lukereative
Copy link

Any further suggestions @robbieaverill on this regarding @dhensby's comment and my reply?

@robbieaverill
Copy link
Contributor

@lukereative yeah I've been thinking about this. I thought about maybe moving the extension point from RequestHandler to something higher in the chain, but then we risk breaking people's code that extends low level classes like this and expects versioned capability.

I personally don't mind the coupling, particularly when there's no great alternative. @dhensby do you have any ideas?

@lukereative lukereative merged commit 8ae0ef0 into silverstripe:1.2 Sep 10, 2018
@lukereative
Copy link

@chillu has asked this be merged as it's impact/critical and since the only pushback is semantic (which doesn't seem to be that avoidable either) rather than anything regarding the function of the fix – we can fix this up later if a better way presents itself.

@chillu chillu deleted the pulls/4.2/dont-apply-stage-link-to-leftandmain branch September 17, 2018 01:46
@ScopeyNZ
Copy link
Contributor

Can we get this merged up to 1 please?

@robbieaverill
Copy link
Contributor

@ScopeyNZ done

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.

5 participants