Problem/Motivation

The Views Handler Manager has a comment:
// @todo This is crazy. Find a way to remove the override functionality.
Which is not very helpful and is causing confusion in #3458099: Views handler loading should respect configuration as per comments from @catch:
https://git.drupalcode.org/project/drupal/-/merge_requests/8588#note_334202

Steps to reproduce

Take a look at ViewsHandlerManager::getHandler()

Proposed resolution

Update the comment and variable name to say what it actually does

Remaining tasks

Provide the merge request

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

Issue fork drupal-3458312

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

scott_euser created an issue. See original summary.

scott_euser opened merge request !8610

scott_euser’s picture

scott_euser commented
Status: Active » Needs review
scott_euser’s picture

scott_euser commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

scott_euser’s picture

scott_euser commented

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

scott_euser’s picture

scott_euser commented

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

scott_euser’s picture

scott_euser commented

Tests passing again

scott_euser’s picture

scott_euser commented

Thanks! Resolved grammar fix

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

scott_euser opened merge request !8682

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

scott_euser’s picture

scott_euser commented
Version: 11.0.x-dev » 11.x-dev
Status: Needs work » Needs review

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

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

scott_euser’s picture

scott_euser commented

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.

  • catch committed 0d55e898 on 11.x 
    Issue #3458312 by scott_euser, andypost: Make the comments in Views...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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