Problem/Motivation

For modules which configuration page is not in a menu, the description of the "Configure" link is extracted using the title_resolver service.
If a custom module uses a "_title_callback" that uses a RouteMatchInterface object and/or if it relies on custom attributes given as default attributes in the route definition, the call to the title_resolver::getTitle() method throws a fatal error.

Proposed resolution

Instead of jumping through hoops to determine the page title for the given configure link:

<a title="really expensive to calculate title with lots of technical implications" href="some/url">Configure</a>

Add context in the form of the module name:

<a href="some/url">Configure<span class="visually-hidden">the {foobar} module</span></a>

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the current code can throw fatal errors, causing WSOD
Issue priority Major because it has impacts on contrib but under rare circumstances
Prioritized changes Bug fix that improves a11y - prioritized
Disruption Not disruptive as Core does not use this for now

User interface changes

Accessibility changes as outlined above.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#26 interdiff.2527546.20.26.txt1.91 KBDuaelFr
#26 modules_list_form_title_resolver_2527546-26.patch6.03 KBDuaelFr
#20 interdiff.2527546.17.19.txt3.32 KBDuaelFr
#20 modules_list_form_title_resolver_2527546-19.patch5.07 KBDuaelFr
#20 fzXQMoVHFqo20qj9ifL4saGxo1_500.jpg47.28 KBDuaelFr
#17 interdiff.2527546.16.17.txt971 bytesDuaelFr
#17 modules_list_form_title_resolver_2527546-17.patch2 KBDuaelFr
#16 modules_list_form_title_resolver_2527546-16.patch2 KBDuaelFr
#13 modules_list_form_title_resolver_2527546-13.patch0 bytesDuaelFr
#13 interdiff.2527546.7.13.txt558 bytesDuaelFr
#7 interdiff.2527546.4.7.txt4.83 KBDuaelFr
#7 modules_list_form_title_resolver_2527546-7.patch4.21 KBDuaelFr
#7 modules_list_form_title_resolver_2527546-7-tests-only.patch2.89 KBDuaelFr
#4 modules_list_form_title_resolver_2527546-4.patch4.27 KBDuaelFr
#4 modules_list_form_title_resolver_2527546-4-tests-only.patch2.94 KBDuaelFr
#1 modules_list_form_title_resolver_2527546-1.patch1.33 KBDuaelFr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Assigned: DuaelFr » Unassigned
Status: Active » Needs review
FileSize
modules_list_form_title_resolver_2527546-1.patch1.33 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Issue tags: +Needs tests

Looks right for me, but we need some tests, given its a bug :(

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented

I'd need some help with that tests :/

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
FileSize
modules_list_form_title_resolver_2527546-4-tests-only.patch2.94 KB
modules_list_form_title_resolver_2527546-4.patch4.27 KB

Let's try this way.
Let me know if it's not enough.

The last submitted patch, 4: modules_list_form_title_resolver_2527546-4-tests-only.patch, failed testing.

The last submitted patch, 4: modules_list_form_title_resolver_2527546-4-tests-only.patch, failed testing.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Issue tags: -Needs tests
FileSize
modules_list_form_title_resolver_2527546-7-tests-only.patch2.89 KB
modules_list_form_title_resolver_2527546-7.patch4.21 KB
interdiff.2527546.4.7.txt4.83 KB

Trying to improve the test I figured out that it would be more clever to extend the system_test module instead of adding something new in the module_test module.

The last submitted patch, 7: modules_list_form_title_resolver_2527546-7-tests-only.patch, failed testing.

The last submitted patch, 7: modules_list_form_title_resolver_2527546-7-tests-only.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

To make it easier to read, the point of the patch is $request->attributes->add($route_object->getDefaults()); this line, the rest is cleanup, replacing strings with constants (very nice cleanup!). So this makes bar: 'baz' in the test pass.

chx’s picture

chx CreditAttribution: chx commented

Ps. I am surprised this is still a thing that needs to happen despite #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API . Someone in the know wants to explain? This shouldn't hold up the patch, the logic is clearly in HEAD already.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented 
+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -307,9 +308,10 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
-          $request->attributes->set('_route', $route_object);
+          $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, $route_object);

This one is important too because '_route' is an incorrect key.
RouteObjectInterface::ROUTE_OBJECT contains '_route_object' instead.

Thank you for your review :)

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
FileSize
interdiff.2527546.7.13.txt558 bytes
modules_list_form_title_resolver_2527546-13.patch0 bytes

Re-reading the code I've seen that I forgot to remove an use that I added during my first tests.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: modules_list_form_title_resolver_2527546-13.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

I think it would be more performant to change it from

<a title="really expensive to calculate title with lots of technical implications" href="some/url">Configure</a>

to

<a href="some/url">Configure<span class="visually-hidden">the {foobar} module</span></a>

More valid for a11y and less expensive to calculate.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Status: Needs work » Needs review
FileSize
modules_list_form_title_resolver_2527546-16.patch2 KB

New patch according to @larowlan suggestion.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
FileSize
modules_list_form_title_resolver_2527546-17.patch2 KB
interdiff.2527546.16.17.txt971 bytes

With @ instead of %.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Looking good - we can now remove the following properties/constructor arguments

routeMatch
menuLinkManager
routeProvider
titleResolver

major cleanup, less coupling++
:)

The last submitted patch, 16: modules_list_form_title_resolver_2527546-16.patch, failed testing.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
FileSize
fzXQMoVHFqo20qj9ifL4saGxo1_500.jpg47.28 KB
modules_list_form_title_resolver_2527546-19.patch5.07 KB
interdiff.2527546.17.19.txt3.32 KB

The last submitted patch, 17: modules_list_form_title_resolver_2527546-17.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Issue tags: +Needs accessibility review

Adding the tag to make sure a11y team agree with my assertions here

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Issue summary: View changes

Updated issue summary for a11y reviewers sake

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 20: modules_list_form_title_resolver_2527546-19.patch, failed testing.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Status: Needs work » Needs review
FileSize
modules_list_form_title_resolver_2527546-26.patch6.03 KB
interdiff.2527546.20.26.txt1.91 KB

Fixed!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Assuming bit agrees
Thanks for your work duaelfr

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented
Issue tags: -Needs accessibility review

Looks good to me. Thanks @DuaelFr & @larowlan

The last submitted patch, 17: modules_list_form_title_resolver_2527546-17.patch, failed testing.

The last submitted patch, 16: modules_list_form_title_resolver_2527546-16.patch, failed testing.

The last submitted patch, 20: modules_list_form_title_resolver_2527546-19.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Doing less is normally a good idea and inject 4 services for a link title feels excessive. And removing untested broken functionality makes sense too. Committed 19167f7 and pushed to 8.0.x. Thanks!

  • alexpott committed 19167f7 on 8.0.x 
    Issue #2527546 by DuaelFr, larowlan, chx: ModulesListForm::buildRow()...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.