Support from Acquia helps fund testing for Drupal Acquia logo

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

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

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

JeroenT’s picture

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

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

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.