Problem/Motivation

Registration uses this pattern:

$handler = $this->entityTypeManager->getHandler('registration', 'host_entity');
$host_entity = $handler->createHostEntity($entity);

This means there is only ever one host_entity handler, that defined for the registration entity type.

It should use this pattern:

$this->entityTypeManager->getHandler($entity->getEntityTypeId(), 'registration_host');
$host_entity = $handler->createHostEntity($entity);

This would allow sites to define different handlers (and thuse host entity wrappers) for different entity types. Which is the whole point of using a handler AFAIK.

Additionally, by namespacing the handler id as registration_host we prevent collision with a module that has its own idea of host entity.

Steps to reproduce

Handlers are documented here:
https://www.drupal.org/docs/drupal-apis/entity-api/handlers

The method signature is:

     * @param string $entity_type_id
     *   The entity type ID for this handler.
     * @param string $handler_type
     *   The handler type to create an instance for.
     *
     * @return object
     *   A handler instance.
     *
     * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
     */
    public function getHandler($entity_type_id, $handler_type);

Proposed resolution

1. Trigger a deprecation error when RegistrationHostEntityHandler is used on the registration entity type. Registration entities can't/shouldn't be hosts.

2. Use hook_entity_type_build() to set RegistrationHostEntityHandler as the default registration_host handler on all entity types.

3. Update all handler uses to use the new pattern.

4. Provide a BC shim using hook_entity_type_alter() and hook_module_implements_alter() in case the host_entity handler is overriden.

Remaining tasks

1. Proof of concept including a single change to use the new pattern in the registration entity class.
2. Fix all the other occurences.

User interface changes

None.

API changes

Using the provided handler for the registration entity type is deprecated.

Data model changes

None.

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:

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw’s picture

Status: Active » Needs review
jonathanshaw’s picture

Issue summary: View changes
jonathanshaw’s picture

Issue summary: View changes
jonathanshaw’s picture

Issue summary: View changes
john.oltman’s picture

Thanks @jonathanshaw, good catch and what you have looks right to me. There are many instances of "getHandler('registration', 'host_entity')" in the module - I am thinking we should correct those to use the new pattern "getHandler($entity->getEntityTypeId(), 'registration_host_entity');" rather than using the deprecated form of the function call.

jonathanshaw’s picture

Status: Needs review » Reviewed & tested by the community

I wonder if there's also value in adding a static helper to HostEntity:

public static function create(Entity interface $entity) {
 $handler = \Drupal::entityTypeManager()->getHander($entity->getEntityTypeId(), 'registration_host_entity');
  return $hander->createHostEntity($entity);
}

john.oltman’s picture

Status: Reviewed & tested by the community » Needs work

Added a commit and tests are now passing. I will try to add a new test this week that proves that the old method triggers a deprecation warning and the new method doesn't. Then I can merge it.

jonathanshaw’s picture

I am having a lot of trouble getting the deprecation test to work on both D10 and D11.

jonathanshaw’s picture

Status: Needs work » Reviewed & tested by the community

I finally got the deprecation tests working.

john.oltman’s picture

Version: 3.1.x-dev » 3.3.x-dev
Status: Reviewed & tested by the community » Needs work

This is really close. Left a couple of comments which may result in code changes. Once those comments are addressed we're good. Thanks for adding the deprecation tests.

jonathanshaw’s picture

Status: Needs work » Needs review

Answered on the MR, the code is all good AFAICS.

john.oltman’s picture

Ah, makes sense, and I do now recall you mentioning this earlier. Will merge shortly.

john.oltman’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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