API Add $action parameter to Controller::Link #5854
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tractorcow
force-pushed
the
pulls/4.0/controller-link-standardise
branch
2 times, most recently
from
d192511
to
3e82492
Compare
|
There seems to be a regression in GridField... looking into it now. |
| public function Link() { | ||
| return get_class($this) .'/'; | ||
| public function Link($action = null) { | ||
| return Controller::join_links(get_class($this), $action, '/'); |
There was a problem hiding this comment.
- Why not just call with get_class +
func_get_args- $action isn't guaranteed to be an action, it'll just be whatever the caller wants - Does it need to finish with a '/'? And if so, can't join_links do that, instead of added it here?
There was a problem hiding this comment.
- We don't want all args, just the one. See tests below; I'm relying on $action being ignored if empty.
- Modifying join_links could have a larger range of unexpected consequences, so I've opted for the least disruptive change.
tractorcow
force-pushed
the
pulls/4.0/controller-link-standardise
branch
from
3e82492
to
f088efc
Compare
|
I've removed the change in behaviour to Form::FormAction() to reduce regressions. This change will now standardise the behaviour of Controller::link(), but will not increase reliance on this standard behaviour. There were too many regressions in GridField due to the original rewrite. |
|
We need a CMS fix too |
|
CMS fix at silverstripe/silverstripe-cms#1570 @dhensby , thanks! |
tractorcow
force-pushed
the
pulls/4.0/controller-link-standardise
branch
from
f088efc
to
9188628
Compare
16 tasks
|
Restarting tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts 1ccdcba
This is necessary to fix some actions in gridfield, where the form Link() action isn't being correctly concatenated to the parent controller.
Prior to this fix, clicking the gridfield header would cause a HTTP error (HTTP request was to
/admin/reports/EditForm/field/Reports, instead of/admin/reports/index/EditForm/field/Reports)The problem is that some controllers need to know if they are being appended to nested controllers, and occasionally need to add the full urlsegment in those cases. The 'home' page and the reports 'index' actions are two examples.