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

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.

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 implies that the view will be deleted if the role is deleted, and the warning won't be triggered.

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.

Or an even bigger change: make connections themselves lazy, so they only try to connect when a query is performed.

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

Opened:
#3441026: Lazily inject the database connection into DbLog
#3441027: Explore whether database connections can be lazily created