Updated: Comment #N
Problem/Motivation
*.info.yml
files support the configure
property, which can contain the name of a route to which a link can be generated for use on the modules page. However, route parameters are not supported, so we can't provide links to any URL path within the system, which is a regression as compared to when the property still accepted a URL path instead of a route name.
For an example, see payment.payment_type.configure. Payments are entities and their bundles (also known as payment types) are provided by modules and cannot be deleted. If a module's main purpose is to provide a payment bundle, its configure link should point to the bundle's configuration page, which is the aforementioned route that requires a static parameter.
Proposed resolution
Add a configure_parameters
property that can specify the parameters needed for a route.
Remaining tasks
None.
User interface changes
None.
API changes
None. The property is optional.
Comment | File | Size | Author |
---|---|---|---|
#45 | drupal_2147689_45.patch | 6.77 KB | Xano |
#22 | interdiff.txt | 4.65 KB | Xano |
#22 | drupal_2147689_22.patch | 6.18 KB | Xano |
Comments
Comment #1
XanoComment #2
RoSk0Comment #5
XanoComment #6
tim.plunkettI didn't add this because it wasn't needed by core or any of the 15 top modules I checked.
If you want to add this, you'll need test coverage.
But also a justification of the use case...
The .info links are by definition static, what dynamic route could they possibly point to?
Comment #7
XanoUpdated the issue summary to address concerns raised in #7.
Dynamic routes are impossible now and will remain impossible with this patch. This patch only introduces support for static route parameters. For a possible use case, see the issue summary.
Comment #8
tim.plunkettBy "dynamic", I really meant "ones with route parameters" :)
The big worry here is about what happens if the placeholder is no longer valid. Like if you tried to link to admin/structure/types/manage/article, but the article content type was deleted.
So this should be used only when the route will always be valid, no matter what.
Comment #9
XanoI opened #2154741: [regression][policy, no patch] Route parameters must be able to be provided along with route names, as this problem does not only surface in
*.info.yml
files.Comment #10
XanoThis same issue can surface in Drupal 7, so this is nothing we haven't had to deal with before, nor is it a regression.
The attached patch contains a test as well.
Comment #11
tim.plunkettTwo lines
...
One line
Which form? Also, public function
Comment #12
XanoDone, except for the empty description. There is no need to repeat what the title already says. See #1941552: [Policy, no patch] Never require descriptions if a title/label is already required.
Comment #13
tim.plunkett#1941552: [Policy, no patch] Never require descriptions if a title/label is already required has no consensus, and hasn't seen any activity in 9 months. Please add a description.
Comment #14
XanoComment #15
tim.plunkettI'm not sure that was the best way to do that.
Comment #16
XanoTest file wasn't strapped in. It fell off along the way.
Comment #17
XanoApologies. The previous interdiff was faulty. The patch is the right one, though.
Comment #19
Xano16: drupal_2147689_16.patch queued for re-testing.
Comment #20
Xano16: drupal_2147689_16.patch queued for re-testing.
Comment #21
Crell CreditAttribution: Crell commentedPlease don't use \Drupal in an object. Get the user injected into the form.
Otherwise this looks good to me.
Comment #22
XanoComment #23
Crell CreditAttribution: Crell commentedThanks, Xano.
Comment #24
alexpottThis looks dodgy to me because it means that the moment system_test is enabled by any test it will not be hidden. So this has the potential to bleed unexpected change into other tests since the only test that requires it not to be hidden is the new test added here.
Comment #25
XanoWhat do you suggest? Also, if another module's tests depend on the visiblity of modules, doesn't that mean those tests are broken?
Comment #26
XanoBump.
Comment #27
XanoComment #29
XanoComment #31
Xano29: drupal_2147689_29.patch queued for re-testing.
Comment #32
XanoThe tests pass, and @alexpott's concern from #24 was addressed.
Comment #33
Crell CreditAttribution: Crell commentedComment #34
XanoRe-roll.
Reject:
Comment #35
XanoRTBC per #33, under the condition that the tests pass.
Comment #36
catchThere's absolutely no documentation added here about 'configure parameters' - ought to be possible to add that somewhere.
Patch also needs a re-roll.
Comment #37
XanoIs the original
configure
parameter even documented anywhere in the code itself? As far as I know this has always only been documented in the online handbook, such as Writing module .info files (Drupal 7.x).Comment #38
XanoCatch and I agreed that this is okay for now, but that the handbook page about writing *info.yml files will need to be updated, and that we will open a follow-up to rename the configure and configure_parameters properties to something more seld-descriptive.
Comment #40
XanoThe follow-up is at #2228357: Rename *.info.yml file "configure" property to "configure_route_name" and it includes a change notice draft.
Comment #41
XanoRe-roll. Reject:
Comment #42
XanoRTBC, under the condition that the tests pass.
Comment #43
webchickHm. :\ TBH I much prefer to see this issue go the other way, where we simply restore the ability to just add paths to the configure parameter. .info files are the easiest thing we have in all of Drupal, and this just further complicates them. Not moving down from RTBC though, since it sounds like catch / alexpott were on board.
Comment #45
XanoRe-roll.
Inconsistency complicates things. This property is used for link generation and that is updated from using paths to route names all over Drupal. Unless we revert that, there is no reason not to commit this patch because of complexity. Besides that, writing *.info.yml files might be easy, they are useless without other code that is much more complex anyway.
Comment #46
XanoRTBC, under the condition that the tests pass.
Comment #47
catchThe advantage of using routes is that if you change the route definition to use a different path, all other code everywhere else works. Wherever we fall back to paths, that's somewhere that modules altering need to be concerned about, or modules updating their own paths have to copy/replace. So it is more clunky but it's not introduced by this issue.
Committed/pushed to 8.x, thanks!