Errors thrown when importing a view using tab links

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

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.

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.

@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 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.

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.

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.

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.

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

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 I would that 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)

Would it be acceptable to do that as a followup?

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 :)

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.

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.

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.