Replace calls to ::expectWarning*() in Drupal\Tests\user\Kernel\Views\HandlerFilterRolesTest

samit.310@gmail.com → made their first commit to this issue’s fork.

Thanks @samit.310@gmail.com, the approach is interesting, but unfortunately not compatible with PHPUnit 10:

PHPUnit 10.5.11 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.3
    Configuration: /builds/issue/drupal-3417066/core/phpunit.xml.dist
    
    .F                                                                  2 / 2
    (100%)
    
    Time: 00:02.629, Memory: 36.00 MB
    
    There was 1 failure:
    
    1) Drupal\Tests\user\Kernel\Views\HandlerFilterRolesTest::testMissingRole
    Failed asserting that exception of type "PHPUnit\Framework\Exception" is
    thrown.
    
    --
    
    1 test triggered 1 warning:
    
    1)
    /builds/issue/drupal-3417066/core/modules/user/src/Plugin/views/filter/Roles.php:104
    The test_user_role role does not exist. You should review and fix the
    configuration of the test_user_name view.
    
    Triggered by:
    
    * Drupal\Tests\user\Kernel\Views\HandlerFilterRolesTest::testMissingRole
     
    /builds/issue/drupal-3417066/core/modules/user/tests/src/Kernel/Views/HandlerFilterRolesTest.php:102
    
    FAILURES!
    Tests: 2, Assertions: 13, Failures: 1, Warnings: 1.

In this set of issues, we need to look at removing the calls to trigger_error(..., E_USER_WARNING) from the runtime code, which is hard to figure out how - each case will have its own solution.

Please follow 📌 Deprecate trigger_error in FileStorage::createDirectory() to allow replacing calls to :::expectWarning*() in Drupal\Tests\Component\PhpStorage\FileStorageTest RTBC for a reference how this could be fixed. I suggest to wait for that to be solved before continuing here.

Opened a 10.3 MR to see if we can deprecate the trigger_error without too much hassle.

I'm not sure we can throw an exception here in calculateDependencies(), instead of triggering a warning. It would be an API change given it's not foreseen in DependentPluginInterface, the ramifications of the places where the exception should be caught is too broad.

Any suggestions?

The warning was first added in #2917006: Views referencing missing roles fail views_post_update_revision_metadata_fields() → to solve some issues in the Drupal 8 upgrade path. Maybe we can just remove it again now? Or, we swap it to a watchdog error and/or the messenger service?

Thanks for taking a look @longwave.

In MR!7229, trying the removal (for D11).

Just removing may lead to WSOD, so we need to control if the role is missing and in that case log a warning, but still continue processing (as is right now).

smustgrave → changed the visibility of the branch 3427171-replace-calls-to to hidden.

smustgrave → changed the visibility of the branch 3427171-10.3 to hidden.

Left a comment on the MR.

Hid the other 2 for clarity.

I think this could be an assert()? It would notify developers but skip at runtime as we can safely ignore it.

How do I even trigger this warning?

I created a role called "test".
I created a view called "test" and added the role filter to it, filtering for the "test" role.
I then tried to delete the "test" role and in the delete modal I see:

Are you sure you want to delete the role test?
This action cannot be undone.

Configuration deletions
The listed configuration will be deleted.

Action

    Add the test role to the selected user(s)
    Remove the test role from the selected user(s)

View

    test

This was originally added in #2917006: Views referencing missing roles fail views_post_update_revision_metadata_fields() → , looks like it only happens in the upgrade path. Not sure what we should do - assert seems like a good option if this mostly useful during updates/development and shouldn't happen at runtime anyway?

Decided that log-and-continue is probably the best option here. We have some precedent in EntityDisplayBase::onDependencyRemoval() which logs a warning when a component depends on a disabled dependency. This uses the default logger channel so I followed the same pattern here, and added test coverage.

- Approach makes sense, seeing there is precedence.
- Code changes make sense (Personally I find the new logged warning a lot more clear than before).
- Tests are green.

The only thing from RTBC-ing this is the CR link, which needs, at least for me, mentioning the new required LoggerInterface $logger, in Roles::__contruct().

I updated the CR.

Thanks @quietone, good enough for me :)

This looks like it could use a service closure - we'll only actually use the logger if there's a problem.

FWIW after doing some profiling last week I don't think it's worth the effort of using closures unless they are on the critical path, constructing a logger object is on the order of microseconds, and the dependent services will already exist. I believe closures are only worthwhile when the constructor has to do a large amount of work or requires a database or network call.

Committed and pushed d708691285 to 11.x and f7ddabe7bb to 10.3.x. Thanks!

the constructor has to do a large amount of work or requires a database or network call.

I think can be the case if the logger is the only/first thing that requires a database connection. This is possible when redis is installed and most caches are warm.

If we are concerned about that I think we should push that decision into DbLog itself, inject the connection as a closure, and only initialise it when we have something to log? Then we don't have to concern ourselves with this every time we need a logger service somewhere.

I think #32 is a good idea, let's open an issue for both of those?

Opened:
📌 Lazily inject the database connection into DbLog Active
📌 Explore whether database connections can be lazily created Active

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

mondrake → closed merge request !7229

mondrake → closed merge request !7005