In context #2225347: [META] Replace calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with static method calls

There's 3 config_test entities:
2 defined by annotated classes
and one defined with config_test_entity_type_alter()

Also patch #0 shows that we can't use static call here

Files: 
CommentFileSizeAuthor
#19 2465751-remove-test-config-load.patch5.95 KBRavindraSingh
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,960 pass(es). View
#16 remove-2465751-16.patch5.95 KBJeroenT
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,494 pass(es), 73 fail(s), and 53 exception(s). View
#16 interdiff.txt2.17 KBJeroenT
#11 remove-2465751-11.patch4.96 KBJeroenT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,582 pass(es). View
#7 2465751-config_test_load-7.patch4.18 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,220 pass(es). View
#7 interdiff.txt888 bytesandypost
#2 2465751-2.patch4.13 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,978 pass(es). View
#2 interdiff.txt4.41 KBandypost
config_test_load-0.patch4.16 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,876 pass(es), 74 fail(s), and 1 exception(s). View

Comments

Status: Needs review » Needs work

The last submitted patch, config_test_load-0.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Configurables, +Configuration system
FileSize
4.41 KB
4.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,978 pass(es). View

Fixed version

andypost’s picture

Title: Remove config_test_load() with static method call » Remove config_test_load() and replace with entity_load
Issue summary: View changes
Berdir’s picture

  1. +++ b/core/modules/config/tests/config_test/src/ConfigTestForm.php
    @@ -33,7 +33,7 @@ public function form(array $form, FormStateInterface $form_state) {
           '#required' => TRUE,
           '#machine_name' => array(
    -        'exists' => 'config_test_load',
    +        'exists' => $this->entity->getEntityType()->getClass() . '::load',
             'replace_pattern' => '[^a-z0-9_.]+',
    

    Dynamic code to get the class name to call it statically so that we can dynamically figure out the entity type still feels a bit weird to me.

    It was hardcoded before, so why not just hardcode it now?

  2. +++ b/core/modules/config/tests/config_test/src/Entity/ConfigQueryTest.php
    @@ -21,6 +21,7 @@
      *   },
      *   config_prefix = "query",
    + *   class = "Drupal\config_test\Entity\ConfigQueryTest",
    

    This won't change anything. The class is automatically set.

andypost’s picture

Yep, that's why I changed approach in #2

Berdir’s picture

This issue doesn't mention config_test_entity_type_alter() anywhere.

This is by design then, there's nothing wrong. We know that ::load() doesn't work when an entity class is re-used multiple times, that's a pretty weird thing to do and I don't see why that makes sense here, we should just define it as a proper annotated class.

There are uses to re-use a class, for example to deal with a dynamic entity types or entity types that are exposed from a third party system, but this isn't a valid use case. Someone was just lazy.

andypost’s picture

Issue summary: View changes
FileSize
888 bytes
4.18 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,220 pass(es). View

Suppose config_test_entity_type_alter() is here for reason to test this alter hook, so maybe better to keep it?

Another question with approach for exists() - I think better to keep this method static, but @tim.plunkett is dislike this in #2091871-27: Add an ConfigEntityForm with an exists() method
So maybe better to use \Drupal::entityQuery() here?

JeroenT’s picture

Patch no longer applied cleanly. Created a new patch and replaced entity_load with the entity storage's load method because those will be marked deprecated for 9.0.0. in #2474151: Mark procedural wrappers in entity.inc as deprecated.

JeroenT’s picture

FileSize
4.18 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,582 pass(es). View
JeroenT’s picture

FileSize
4.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,582 pass(es). View

Wrong patch. This is the right one.

JeroenT’s picture

tim.plunkett’s picture

  1. +++ b/core/modules/config/tests/config_test/src/ConfigTestForm.php
    @@ -33,7 +33,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +        'exists' => [get_class($this), 'exists'],
    

    Just use [$this, 'exists'], and then everything will be cleaner below

  2. +++ b/core/modules/config/tests/config_test/src/ConfigTestForm.php
    @@ -142,4 +142,25 @@ public function save(array $form, FormStateInterface $form_state) {
    +    return (bool) \Drupal::entityQuery($entity->getEntityTypeId())
    

    Either inject entity query, or use $this->entityManager->getStorage($entity_type_id)->getQuery()

JeroenT’s picture

@tim.plunkett, hmm, I think it was done this way, because exists is a static method and $this is not available in it.

tim.plunkett’s picture

Yes, and I'm saying make it non-static, like the other exists() methods in forms.

JeroenT’s picture

FileSize
2.17 KB
5.95 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,494 pass(es), 73 fail(s), and 53 exception(s). View

Alright, changed the method to a non-static method.

Patch attached.

Status: Needs review » Needs work

The last submitted patch, 16: remove-2465751-16.patch, failed testing.

tim.plunkett’s picture

Thanks!

  1. +++ b/core/modules/config/tests/config_test/src/ConfigTestForm.php
    @@ -16,6 +18,32 @@
    +   * Constructs a new ConfigImportForm.
    

    Wrong class name

  2. +++ b/core/modules/config/tests/config_test/src/ConfigTestForm.php
    @@ -16,6 +18,32 @@
    +    $this->$entityQuery = $entity_query;
    

    $this->entityQuery

RavindraSingh’s picture

Status: Needs work » Needs review
FileSize
5.95 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,960 pass(es). View

Fixed mentioned in #18

Not uploading any interdiff as it is just correction suggested by @tim.plunkett

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

By definition this affect test only code and therefore is permitted in beta. Committed aecc5a4 and pushed to 8.0.x. Thanks!

  • alexpott committed aecc5a4 on 8.0.x
    Issue #2465751 by andypost, JeroenT, RavindraSingh, tim.plunkett, Berdir...

Status: Fixed » Closed (fixed)

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