Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Currently, a blank parameters array is passed into $form_state->setRedirect(), which doesn't allow dynamic routes.
Getting raw parameters from RouteMatch will allow dynamic routes and at the same time does not affect routes without required parameters.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
None
API changes
\Drupal\views\Plugin\views\field\BulkForm constructor changes. See https://www.drupal.org/node/3115868
Data model changes
None
Release notes snippet
Comment | File | Size | Author |
---|
Issue fork drupal-2901412
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
grathbone CreditAttribution: grathbone as a volunteer commentedPopulated route parameters from raw parameters of current RouteMatch and passed the array into $form_state->setRedirect()
Comment #4
grathbone CreditAttribution: grathbone as a volunteer commentedComment #5
grathbone CreditAttribution: grathbone as a volunteer commentedRerolled for changes in 8.5
Comment #6
grathbone CreditAttribution: grathbone as a volunteer commentedComment #7
LendudeThis sounds more like a task or a feature request. Not sure what is broken here?
We'd need some kind of test to make sure this does what we want it to.
We'd want to use dependency injection for this.
Comment #11
claudiu.cristeaFixed #7 and added tests.
Comment #13
claudiu.cristeaFixing the unit tests & coding standards.
Comment #14
LendudeSo the actual change comes down to this:
Nice test coverage, don't see any nits to pick.
This would be hard to accomplish in contrib, so it makes sense to me to add this to core. Just not sure if it is 8.9.x eligable.
Comment #15
pfrenssenPatch looking good, RTBC+1.
Comment #17
xjmThanks for your work on this!
The patch doesn't apply to 9.1.x, so we need a new version. Thanks!
Comment #18
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have done patch #13 on Drupal 9.1.x.
Comment #25
claudiu.cristeaComment #27
claudiu.cristeaComment #29
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commentedJust a note here for someone that is blocked by it, the patch does not apply to 9.3.14 after https://www.drupal.org/project/drupal/issues/2917239 was merged which conflicts with the current patch.
Anyone that is on 9.3.14 should include the patch in https://www.drupal.org/project/drupal/issues/2917239 too.
Comment #30
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #32
claudiu.cristeaComment #33
pfrenssenLooking good, patch has been updated to work with D10, tests are passing.
Comment #34
quietone CreditAttribution: quietone at PreviousNext commentedI'm triaging RTBC issues. I re-read the IS and the comments. I didn't find any unanswered questions.
I read the patch (not a review) and found that the version numbers for the deprecation message are incorrect. I am setting to NW for that. The same is true for the Change record. I was thinking the title should have more details. More like this, "\Drupal\views\Plugin\views\field\BulkForm::__construct now requires \Drupal\Core\Routing\ResettableStackedRouteMatchInterface" Although, PHPStorm is complaining that
ResettableStackedRouteMatchInterface does not match the declaration.
The diff applies to 11.x but there is no MR against 11.x. So, we should get it testing on 11.x.
I added the standard template to the IS in case it was needed. Setting to NW for the above.
Comment #35
quietone CreditAttribution: quietone at PreviousNext commentedI think add is better, since this is adding a new argument.
Comment #37
claudiu.cristeaThank you:
The main scope of this issue is not to add a new argument to that constructor but to forward/pass the bulk form route parameters to the confirmation form. However, I think using "add" would not make any difference.
Moving back to RTBC.
Comment #43
alexpottI've changed the deprecation to be 10.3.0 as this has missed 10.2
Comment #44
alexpottFixing issue credit
Comment #45
alexpottCommitted 79a07a4 and pushed to 11.x. Thanks!