Refactor ImageFieldCreationTrait to support entity types other than nodes

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.

Trying a new approach.

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

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Appears to have test failures. Also MR was draft was that on purpose?

Hiding patches as fix is in MR.

Yeah, I wanted to see what people thought of the approach I've taken for BC.

Any feedback?

No feedback here or on slack, so updated all the existing calls.

That's a really clever way to handle inserting a new argument I think.

I'm leaving it in needs work since there are so many failures, though that might be related to the gitlab update.

Let me rerun it and see if it passed before moving status.

After rerunning tests they are failing

Argh I did a search and replace of the whole codebase! I could have SWORN I did that, and I don't see the commit!!!!

In case I have to do this again, the search and replace is:

>createImageField\(([^,]+), ([^,]+)\)
>createImageField($1, 'node', $2)

Yeah I saw your comment, maybe you pushed it to a different branch or fork?

Anyway, there seem to be some relevant failing tests still. For example:

core/modules/image/tests/src/Functional/ImageOnTranslatedEntityTest.php
 There was 1 error:
    
    1)
    Drupal\Tests\image\Functional\ImageOnTranslatedEntityTest::testSyncedImages
    Symfony\Component\Validator\Exception\UnexpectedValueException: Expected
    argument of type "string", "array" given

Remaining self deprecation notices (1)
    
      1x: Calling
    Drupal\Tests\image\Kernel\ImageFieldCreationTrait::createImageField()
    without the $entity_type argument is deprecated in drupal:10.3.0 and this
    argument will be required in drupal:11.0.0. See
    https://www.drupal.org/node/3441322
        1x in ImageOnTranslatedEntityTest::testSyncedImages from
    Drupal\Tests\image\Functional

There are a bunch of other related to images, I suspect there is an indirect call missing in the testing somewhere.

> There are a bunch of other related to images, I suspect there is an indirect call missing in the testing somewhere.

Nothing as complicated as that, just that last night I was tired and I forgot to also replace the

>createImageField\(([^,]+), ([^,]+),
>createImageField($1, 'node', $2,

uses as well :/

Looks good now!

I've also confirmed all instances have been updated now.

Sorry forgot to mention I also looked at the CR, looks good too.

Sorry I didn't catch this earlier, but this probably needs a test adding an image field to at least one other entity type now to prove it actually works and to ensure it doesn't regress in the future. As of now every instance of it is only calling node entities.

After discussing this on Slack.

@joachim pointed out the following: traits don't have dedicated test coverage.
Also this is a refactor not a bug fix so tests may not be required.

Waiting for tests to pass then I'll update status again.

Made a 10.3.x branch, so we have:

- https://git.drupalcode.org/project/drupal/-/merge_requests/7739 - 10.3.x
- https://git.drupalcode.org/project/drupal/-/merge_requests/7207 - 11.x with deprecation removed

Neat! Thanks for your thoughtfulness on BC here. Saving issue credits. Haven't started the code review yet.

Which/where was the pipeline job showing the demonstrative test fails as mentioned in #20?

Weirdly wrong tag.

OK, the approach is perfect. Looking into why the 11.x version doesn't have pipelines/does not appear to be mergeable when this is correctly filed against 11.x.

Reviewed locally with git diff --color-words and committed to 11.x.

Meanwhile, I accepted my comment fix for an on-commit change to the backport version.

Committed the backport version to 10.3.x and published the CR. Thanks everyone!

xjm → closed merge request !7739

xjm → closed merge request !7207