10.2: EntityReferenceTestTrait to EntityReferenceFieldCreationTrait rector using RenameClassRector

[bbrala]

Description

Fixes: https://www.drupal.org/node/3401941

When renaming it does not remove the old use statement. This should not error, but is ugly.

Can only be avoided if we start applying codestyle to the files. Which seem overkill. Although something like this could work maybe? rector-src/vendor/symplify/easy-coding-standard/config/set/common/namespaces.php

To Test

Functional tests

Drupal.org issue

Provide a link to the issue from https://www.drupal.org/project/rector/issues. If no issue exists, please create one and link to this PR.

[mglaman]

It's great we can use Rector\Renaming\Rector\Name\RenameClassRector, however this will break anything <10.2.

The way Drupal core did this even broke contrib as it wasn't properly deprecated.

I don't even know the best way to handle this via Rector as it's a breaking change to use the new trait.

[bbrala]

I've been looking into how i could use two traits, but the ways to do it are very very very very ugly.

The sad thing is, the method could have been static...

  /**
   * Creates a field of an entity reference field storage on the specified bundle.
   *
   * @param string $entity_type
   *   The type of entity the field will be attached to.
   * @param string $bundle
   *   The bundle name of the entity the field will be attached to.
   * @param string $field_name
   *   The name of the field; if it already exists, a new instance of the existing
   *   field will be created.
   * @param string $field_label
   *   The label of the field.
   * @param string $target_entity_type
   *   The type of the referenced entity.
   * @param string $selection_handler
   *   The selection handler used by this field.
   * @param array $selection_handler_settings
   *   An array of settings supported by the selection handler specified above.
   *   (e.g. 'target_bundles', 'sort', 'auto_create', etc).
   * @param int $cardinality
   *   The cardinality of the field.
   *
   * @see \Drupal\Core\Entity\Plugin\EntityReferenceSelection\SelectionBase::buildConfigurationForm()
   */
  protected function createEntityReferenceField($entity_type, $bundle, $field_name, $field_label, $target_entity_type, $selection_handler = 'default', $selection_handler_settings = [], $cardinality = 1) {
    // Look for or add the specified field to the requested entity bundle.
    if (!FieldStorageConfig::loadByName($entity_type, $field_name)) {
      FieldStorageConfig::create([
        'field_name' => $field_name,
        'type' => 'entity_reference',
        'entity_type' => $entity_type,
        'cardinality' => $cardinality,
        'settings' => [
          'target_type' => $target_entity_type,
        ],
      ])->save();
    }
    if (!FieldConfig::loadByName($entity_type, $bundle, $field_name)) {
      FieldConfig::create([
        'field_name' => $field_name,
        'entity_type' => $entity_type,
        'bundle' => $bundle,
        'label' => $field_label,
        'settings' => [
          'handler' => $selection_handler,
          'handler_settings' => $selection_handler_settings,
        ],
      ])->save();
    }
  }

We could do some magic with an added method and an anonimous class, which loads based on which class exists. Especially since that trait is not using the object context at all.

Then it could be even backwards compatible. It just uses ugly code then.

Somthings like

protected function createEntityReferenceField($entity_type, $bundle, $field_name, $field_label, $target_entity_type, $selection_handler = 'default', $selection_handler_settings = [], $cardinality = 1) {
    if(trait_exists('Drupal\Tests\field\Traits\EntityReferenceFieldCreationTrait')){
      $caller = new class() { use \Drupal\Tests\field\Traits\EntityReferenceFieldCreationTrait; };
    } else {
       $caller = new class() { use \OldTrait; };
     }

    
      $caller->createEntityReferenceField($entity_type, $bundle, $field_name, $field_label, $target_entity_type, $selection_handler, $selection_handler_settings, $cardinality);
}

This might work, but the code is not very pretty.

[mglaman]

It'd probably be easier to add a class alias at the top of the class for the trait.

[bbrala]

I don't understand what you mean.

[bbrala]

Ok something like

<?php 
trait Foo {}
class_alias("Foo","Bar");
echo trait_exists("Bar") ? 'yes' : 'no'; 
?>
//yes

But wrapped more sanely

[bbrala]

Ok, something like this should work.

/**
 * @description In order to support Drupal versions before 10.2 we need to alias the EntityReferenceFieldCreationTrait to EntityReferenceTestTrait.
 */
if (!class_exists(\Drupal\Tests\field\Traits\EntityReferenceFieldCreationTrait::class)) {
  class_alias(\Drupal\Tests\field\Traits\EntityReferenceTestTrait::class, \Drupal\Tests\field\Traits\EntityReferenceFieldCreationTrait::class);
}

class Test {

  use EntityReferenceFieldCreationTrait;

}

$ref = new ReflectionClass(Test::class);

var_dump($ref->getTraits());

Would probably not use the RenameClassRector but should use the service. That way renaming is easy, but we are targetting a trait right now, so might reduce the scope initially to keep it simple.

Probably:

1. Only target classes
2. Check their trait use for the deprecated trait
3. If found add the class alias stuff (hopefully we can while at a class node)
4. Use RenamedClassesDataCollector to rename the class, hopefully.

Few things im not sure about yet:

1. When should the class_alias be removed when using upgrade bot. And does this mean min version will be upped to 10.2 even though this could also work in earlier version.
2. Adding the code above the class might not work.
3. Getting down into the class and renaming the use statement might be a hassle.

Also important: 10.2 was fixed in regards to keeping the old class see https://www.drupal.org/node/3403491

[mglaman]

Yeah, like that. But only in tests using the trait – I have no idea how hard that makes it. Maybe target Class and then use PHPStan scope/tools to detect traits used.

When should the class_alias be removed when using upgrade bot. And does this mean min version will be upped to 10.2 even though this could also work in earlier version.

Good Q. Maybe when D10 is EOL.

Adding the code above the class might not work.

It should. See this abomination https://git.drupalcode.org/project/acquia_connector/-/blob/4.x/src/Event/EventBase.php?ref_type=heads

[bbrala]

Haha yeah I understand the code will work. But mostly if I can get rector to do it :)

But I'll have a look, and perhaps we shouldn't care too much about forcing ^10.1 || ^11 in the modules. Although unneeded.

[bbrala]

All code that will be for 10.x deprecation will have that anyways, since deprecationhelper is only available since then.

[agentrickard]

@mglaman I fixed the merge conflict here and tests pass locally. Could use another review.

[mglaman]

@bbrala should we wrap this with a BC protection? I guess then we'd need to extend RenameClassRector and a config object to do so :/ or is there an easier way?

[bbrala]

Yeah we need to wrap. I've wanted to wrap for a few times now, but haven't taken the time to see if its ewasily possible.

You'd need to do quite a few weird things to make sure things are instantiated correctly. Would open up loads of easy things like this rector though.

[bbrala]

Since all classes in rector are final, there is no extending.

[agentrickard]

I believe this PR is held up for the BC refactor?

[bbrala]

Yeah pretty much. Just feels like a lot of effort for 10.1 vs 10.2... Although the pattern might be usefull later.

[bbrala]

Ok, visited this again. Something generic is very hard. Something expanding on what RenameClassRector does AND hooking into the file is kinda hard.

So guess it needs a specific rector for this change, which is sad. And my time is up for today ;x

[agentrickard]

I believe this PR is held up for the BC refactor?

[agentrickard]

@bbrala @mglaman This PR is green again. Are we good to merge?