Make the comments in Views handler Manager around overrides clearer

If you want to find the two uses of this override in core, easiest is to search the codebase for Views::handlerManager( as the 'type' passed is dynamic. Anyways easy to spot the two cases where the 2nd arg is passed via a phpstorm code search for that. Both cases trace back to Drupal\views\Plugin\views\query\Sql::getAggregationInfo() (and would be similar for something other than Sql if we supported aggregation with another database engine).

I find $override_handler_plugin_id over-complicated - handler is always plugin and the method name already suppose to return handler getHandler() so $override_plugin_id is more reasonable

Thanks for the feedback! I also added the return typehint ViewsHandlerInterface + renamed the variable as suggestion.

Retrying pipeline, the failed block cache test seems unrelated and passes locally when I run it, so I think its a gitlab ci anomaly.

Tests passing again

Thanks! Resolved grammar fix

Small request can MR be updated for 11.x vs 11.0.x

scott_euser → changed the visibility of the branch 3458312-overrride-comment-views-handler to hidden.

Thanks for flagging, updated to 11.x (issue & MR)

Comment reads fine, not 100% sure about the variable name if there is any BC for that but going to mark.

Thanks! Re variable name, since its not an interface change, even if someone is for some reason extending this (I can't see why), I believe it would not cause a breaking change if they have named their variable differently.

This is a good improvement for a very confusing and complicated bit of code.

Wondered about the bc implications of adding the type hint to ViewsHandlerManager but: 1. overriding this seems extremely unlikely 2. downstream code could just add the type hint to be compatible with all Drupal versions again.

Committed/pushed to 11.x, thanks!

catch → closed merge request !8682

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