There have been many change to the link generation mechanisms at DrupalCon Amsterdam

There are many issues connected together , the following issue is a good reference point

#2343651: Remove most remaining l() calls

The link generation in \Drupal\views_ui\Controller\ViewsUIController is broken, reportPlugins()
This little fix seems to have escaped all the changes.

It is hampering the development in

#2347077: Write an admin crawler test

CommentFileSizeAuthor
link-0.patch783 bytesmartin107

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Thanks for filing this!

So the problem is that without this patch, if you go to admin/reports/views-plugins you get:

Error

Recoverable fatal error: Argument 2 passed to Drupal\Core\Controller\ControllerBase::l() must be an instance of Drupal\Core\Url, string given, called in /home/jhodgdon/gitrepos/drupal_msq8/core/modules/views_ui/src/Controller/ViewsUIController.php on line 123 and defined in Drupal\Core\Controller\ControllerBase->l() (line 40 of core/lib/Drupal/Core/Routing/LinkGeneratorTrait.php).

With this patch, you get the desired report, and the links work (it displays views plugins with links to views that use each one).

Thanks for filing the issue and making a patch!

And as a note, this bug was discovered by the test in #2347077: Write an admin crawler test... we might want to have this test as part of our test suite... might save us headaches!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Priority: Normal » Major

Also as this is a fatal error, the issue is at least Major if not Critical (I am not sure that the Views Plugin Report is a critical part of Drupal, so not marking Critical, even though the report totally doesn't work without this simple one-line fix).

martin107’s picture

martin107 commented
Issue tags: +Quickfix

An admin page is broken on the beta...

So while celebrating a major milestone... I am wistfully pining for beta-2.. what is wrong with me!!!

This is a small localised change.

It fits a common pattern, which maybe be in the short term memory of reviewers and committers.

QuickFix++

geodaniel’s picture

geodaniel commented

This looks good to me. Just closed the duplicate I created at #2348057: Error when loading admin/reports/views-plugins on fresh install.

alexpott’s picture

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

Committed 51a6720 and pushed to 8.0.x. Thanks!

  • alexpott committed 51a6720 on 
    Issue #2347987 by martin107: Fixed Broken l() in reportPlugin.
dawehner’s picture

dawehner
Primary language German
commented

What about tests?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

If my Admin Crawler patch gets in, it will test this page. :)

martin107’s picture

martin107 commented

#2347077: Write an admin crawler test

if committed will visit all admin pages and perform elementary checks...so this gross failure will be picked up there.

Perhaps you had something more specific in mind? ... that might even be an offer of help :)

I appreciate that now we are in beta ... committers are rightfully being instructed to tighten up on code quality..

Is you comment that the controller needs integration testing .. alternatively there are lots of complex foreach loops in and reportPlugin / reportFields
that might benefit from a unit test passing mocked data through them?

xjm’s picture

xjm
she/her
Primary language English
commented
Related issues: +#2347077: Write an admin crawler test

Thanks @martin107. It's not a matter of the fact that we are in beta or not; our expectations of code quality are the same throughout the release cycle. It's a core policy that every bugfix should come with a regression test when it is in committed. See: https://www.drupal.org/core-gates#testing

#2347077: Write an admin crawler test sounds like a good solution in this case. It's just not usual for patches to be committed without mentioning it.

Status: Fixed » Closed (fixed)

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