Problem/Motivation

When working on the Events recipe, I notice that there are consistently errors thrown when importing the provided view. For example:

Warning: Undefined array key "events.page_1" in Drupal\views\Plugin\Derivative\ViewsLocalTask->getDerivativeDefinitions() (line 82 of core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php).
Drupal\views\Plugin\Derivative\ViewsLocalTask->getDerivativeDefinitions(Array) (Line: 101)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array) (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 337)
Drupal\Core\Plugin\DefaultPluginManager->findDefinitions() (Line: 213)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 181)
Drupal\Core\Menu\LocalTaskManager->getDefinitions() (Line: 206)
Drupal\Core\Menu\LocalTaskManager->getLocalTasksForRoute('view.frontpage.page_1') (Line: 290)
Drupal\Core\Menu\LocalTaskManager->getTasksBuild('view.frontpage.page_1', Object) (Line: 358)
Drupal\Core\Menu\LocalTaskManager->getLocalTasks('view.frontpage.page_1') (Line: 251)
Drupal\navigation\NavigationRenderer->getLocalTasks() (Line: 288)
Drupal\navigation\NavigationRenderer->hasLocalTasks() (Line: 224)
Drupal\navigation\NavigationRenderer->removeLocalTasks(Array, Object) (Line: 162)
navigation_block_build_local_tasks_block_alter(Array, Object, NULL) (Line: 552)
Drupal\Core\Extension\ModuleHandler->alter('block_build', Array, Object) (Line: 65)
Drupal\block\BlockViewBuilder->viewMultiple(Array, 'full', NULL) (Line: 32)
Drupal\block\BlockViewBuilder->view(Object) (Line: 152)
Drupal\block\Plugin\DisplayVariant\BlockPageVariant->build() (Line: 270)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 128)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object) (Line: 246)
Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}(Object, 'kernel.view', Object) (Line: 206)
Symfony\Component\EventDispatcher\EventDispatcher->callListeners(Array, 'kernel.view', Object) (Line: 56)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch(Object, 'kernel.view') (Line: 188)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

(the error above is thrown twice, once for each display)

Warning: Undefined array key "events.page_2" in Drupal\views\Plugin\Derivative\ViewsLocalTask->alterLocalTasks() (line 122 of core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php).
Drupal\views\Plugin\Derivative\ViewsLocalTask->alterLocalTasks(Array) (Line: 812)
views_local_tasks_alter(Array, NULL, NULL) (Line: 552)
Drupal\Core\Extension\ModuleHandler->alter('local_tasks', Array) (Line: 386)
Drupal\Core\Plugin\DefaultPluginManager->alterDefinitions(Array) (Line: 341)
Drupal\Core\Plugin\DefaultPluginManager->findDefinitions() (Line: 213)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 181)
Drupal\Core\Menu\LocalTaskManager->getDefinitions() (Line: 206)
Drupal\Core\Menu\LocalTaskManager->getLocalTasksForRoute('view.frontpage.page_1') (Line: 290)
Drupal\Core\Menu\LocalTaskManager->getTasksBuild('view.frontpage.page_1', Object) (Line: 358)
Drupal\Core\Menu\LocalTaskManager->getLocalTasks('view.frontpage.page_1') (Line: 251)
Drupal\navigation\NavigationRenderer->getLocalTasks() (Line: 288)
Drupal\navigation\NavigationRenderer->hasLocalTasks() (Line: 224)
Drupal\navigation\NavigationRenderer->removeLocalTasks(Array, Object) (Line: 162)
navigation_block_build_local_tasks_block_alter(Array, Object, NULL) (Line: 552)
Drupal\Core\Extension\ModuleHandler->alter('block_build', Array, Object) (Line: 65)
Drupal\block\BlockViewBuilder->viewMultiple(Array, 'full', NULL) (Line: 32)
Drupal\block\BlockViewBuilder->view(Object) (Line: 152)
Drupal\block\Plugin\DisplayVariant\BlockPageVariant->build() (Line: 270)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 128)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object) (Line: 246)
Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}(Object, 'kernel.view', Object) (Line: 206)
Symfony\Component\EventDispatcher\EventDispatcher->callListeners(Array, 'kernel.view', Object) (Line: 56)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch(Object, 'kernel.view') (Line: 188)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Knowing that this recipe is very similar to the Smart Date Starter Kit (which is available as both a module and a recipe), I installed the module and then replaced the supplied view config file with a copy of the view file from the Events recipe (to make sure there would be no differences in the configuration). When I enabled the module, there were no errors.

Steps to reproduce

On a fresh install of Drupal 11, composer require and then apply the Events recipe. Notice that when you visit the website after the recipe is successfully applied, errors are shown like the ones above.

Proposed resolution

I'm not sure why the errors are only created when configuration is imported through the recipes system, so additional investigation is required.

Issue fork drupal-3464160

Command icon 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:

Issue fork distributions_recipes-3464160

Command icon 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:

About issue forks

Comments

mandclu created an issue. See original summary.

thejimbirch’s picture

thejimbirch commented
Issue tags: +Recipes initiative
bhuvaneshwar’s picture

bhuvaneshwar commented
Assigned: Unassigned » bhuvaneshwar

mandclu opened merge request !161

mandclu’s picture

mandclu commented
Project: Recipes Initiative » Drupal core
Component: Code » views.module

mandclu changed the visibility of the branch 3464160-errors-thrown-when to hidden.

mandclu opened merge request !9157

mandclu’s picture

mandclu commented
Assigned: bhuvaneshwar » Unassigned
Status: Active » Needs review

Adding a nullsafe fallback to empty strings resolves the errors that are currently thrown when applying the events recipe. I also verified that the configuration is still imported as expected.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Definitely believe this will need test coverage

Wonder if a configuration is wrong. Only mention as not sure putting a null check is enough or is masking a potentially large/different issue.

mandclu’s picture

mandclu commented

@smustgrave I would be happy to write the tests, but would appreciate a little guidance on how to test these. All this change does is allow the code to more gracefully handle the situation where a particular view / display combination isn't found in the array returned by $this->state->get('views.view_route_names'). In both cases the next line is a check whether a retrieved route name matches a constructed plugin id (which will never be empty), so this check effectively handles the case where the expected route name can't be found. Without the fix here, the same checks catch the mismatch and trigger a return of the function, but throw a PHP warning along the way. I would argue that the proposed fix doesn't alter the logic of either function, it just allows it to more gracefully handle a reproducible use case when creating a view via a recipe.

It's possible there is a deeper issue around the order in which the Recipes code handles elements of the provided view configuration, but after applying the recipe and clearing the cache, the provided local tasks work as expected. It's also worth noting that the identical configuration has been previously installable as part of the Start Date Starter Kit, so several hundred sites are using this configuration, and when it is installed as the configuration provided by a module, no errors are thrown.

smustgrave’s picture

smustgrave commented

Do we believe this is incorrect config. Should there be a log made when $this->state->get('views.view_route_names') isn't found.

Even more should it be a deprecation that in D12 this will throw an error.

mandclu’s picture

mandclu commented

I think it's worth reiterating that in the situation where I can consistently reproduce these warnings being output, the site still works as expected. Suppressing the warnings requires twelve characters, and the subsequent code still handles the unfound route name.

As to the larger question whether or not an unfound route name should trigger an error, that sounds a valid topic to explore, but I would suggest better handled as a separate issue, particularly since it could have the potential for far-reaching implications.

nicxvan’s picture

nicxvan commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

I think this might not need tests according to the new testing guidelines:

https://www.drupal.org/about/core/policies/core-change-policies/core-gat...

Both of these are true:

  • The issue has clear ‘Steps to reproduce’ in the Issue Summary.
  • The fix is 'trivial' with small, easy to understand changes.

And the following questions:

  1. Is the fix is easy to verify by manual testing? Yes
  2. Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc. Not sure if local tasks are internal
  3. Is the fix achieved without adding new, untested, code paths?Yes
  4. Is an explicit 'regression' test needed?No
  5. Is it easy for someone who did not work on the original bug report to add the test coverage in a followup issue?Yes
  6. Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue?No
  7. If this fix is committed without test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed?Yes

4 out of 7 is the majority, I don't think this needs tests.

I'm not sure if we can add the tag that it needs no tests or just core maintainers can so I'm just RTBC and leaving my observations and removing needs tests.

smustgrave’s picture

smustgrave commented

Will still mention feels as though we are probably masking another issue.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

I agree with @smustgrave this is most likely just masking an issue. If you have a tab and no route there is something wrong with your install, so why are we suppressing the warning, when it seems to be doing it's job and warning you that there is something wrong?

And yeah it seems most likely a timing issue that $this->state->get('views.view_route_names') is not properly populated yet or something along those lines. But if that is the case we need to fix the timing issue and not just ignore the error.

But I think we need to properly investigate the root cause and then we can make a decision as to the proper fix (which might turn out to be to suppress the error)

nicxvan’s picture

nicxvan commented

Would it be acceptable to do that as a followup?

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

In my opinion, no, because we have no idea what we are 'fixing' here. But I'll leave it RTBC for others to look at, happy to be wrong :)

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs work

I'm sorry, I agree with @Lendude and @smustgrave here; the fix may well be to suppress the warning, but without knowing why, this patch is incomplete. Test coverage may in fact be needed if there's actually a bug here.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 3464160-19.patch610 bytes

Based on some conversation with @mandclu in Slack, I'm starting to think this is a problem specific to the command-line recipe runner. Tracing through core, what I'm seeing is that the router table should be rebuilt at the end of the request, which would avoid this error. But the thing is that RecipeCommand never actually terminates the "request" properly, and therefore the router is never rebuilt (until the next cache clear, at least).

I suspect the fix here is to have RecipeCommand do a proper request termination, with all destructable container services notified (which would trigger a router rebuild). Patch attached for folks to test with.

mandclu’s picture

mandclu commented
Title: Errors thrown when importing a view using tab links » Warnings output when importing a view using tab links
mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

I had a user report this in a contrib. module of mine outside of the recipes work flow. I don't have any additional steps to reproduce yet, but I think it could also occur not just in recipes install but some other fatal error on module install.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.