EntityTypeManager::getHandler() doesn't allow for nesting

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased the branch.

Removing tag as this has tests.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The branch is up to date AFAICT.

Merge request is open against 9.5 branch, the target branch needs to be updated on the MR

I can't see where to change it in gitlab.

ah yes, it's a gitlab limitation, only the user who created the merge request (and possibly committers) can change a MR target (or close it). Only way here is uploading a patch or opening a new MR.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

the bot will keep putting it back, if you want to avoid it you can use the "no-needs-review-bot" tag and this issue will be ignored. useful if you want to keep a NR status on a work in progress patch/MR.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Change looks good to me. This will need a change record to announce the new parameter that the interface is using.

Written the CR.

Perfect.

Example was a great addition

This looks good, I just have a question in relation to other patterns in core, such as $form_state->getValue() where we accept either a string or an array for the key, and use NestedArray::get to pluck the item.

I wonder why we didn't use that in getHandlerClass instead of the optional nested param.

I wonder if we could explore whether that would be possible as it would be cleaner than the optional argument in my opinion, and more consistent with other places in core that deal with getting things out of keyed arrays.

Obviously this would also allow arbitrary nesting levels if someone was wanted to support a three or more layers deep (🤢)

Main question would be can we do it with a BC layer and deprecate the nested param.

Also, not sure this is a bug, there's an existing way to do this with getHandlerClass, so moving to a task.

Moving to NR to discuss the NestedArray idea.

> I wonder why we didn't use that in getHandlerClass instead of the optional nested param.

The consistency with form state would be nice, definitely. I did consider it when working on this issue, but I figured that there's the existing pattern for form and route handlers, and I wanted to minimize disruption.

Would performance be a potential problem? FormStateValuesTrait::getValue() does this for every call:

> $value = &NestedArray::getValue($this->getValues(), (array) $key, $exists);

Forms are less of a performance concern AFAIK as it's editors and admins who use forms rather than the general public, and users expect forms to take a bit more time than a page load.

Whereas getting an entity handler is something that happens all the time. I guess in our implementation we could bypass NestedArray if $key is a string, as our majority case is that it's a string -- whereas with form state it's probably not as a lot of forms use nested elements.

I'd be happy to move the discussion around passing an array to a follow up fwiw

Marking now but a follow up should be created per #24

> I'd be happy to move the discussion around passing an array to a follow up fwiw

The problem with doing that is we'd be adding to the API here and then removing the new behaviour soon after in a follow-up.

I was just coming back to post the same thing @joachim.

Ok. In which case, we need to figure out whether single keys need special handling for performance, or whether we can pass the whole thing to NestedArray the way FormState does.

Also, the local cache is keyed with the entity ID at the *end* like this:

> $this->handlers[$handler_type][$entity_type_id]

which means making the $parents for NestedArray is going to be fiddly. I'm temped to switch it to the other way round, so we can do:

NestedArray::getValue($this->handlers[$entity_type_id], $parents)

Obviously $this->handlers is a protected property, and the BC policy says it's internal. But it may still trip up people who've overridden the service with a child class.