Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#19 | 2465751-remove-test-config-load.patch | 5.95 KB | RavindraSingh |
#16 | remove-2465751-16.patch | 5.95 KB | JeroenT |
#16 | interdiff.txt | 2.17 KB | JeroenT |
#11 | remove-2465751-11.patch | 4.96 KB | JeroenT |
| |||
#7 | 2465751-config_test_load-7.patch | 4.18 KB | andypost |
Comments
Comment #2
andypostFixed version
Comment #3
andypostComment #4
BerdirDynamic 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?
This won't change anything. The class is automatically set.
Comment #5
andypostYep, that's why I changed approach in #2
Comment #6
BerdirThis 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.
Comment #7
andypostSuppose
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() methodSo maybe better to use
\Drupal::entityQuery()
here?Comment #9
JeroenTPatch 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.
Comment #10
JeroenTComment #11
JeroenTWrong patch. This is the right one.
Comment #12
JeroenTComment #13
tim.plunkettJust use
[$this, 'exists']
, and then everything will be cleaner belowEither inject entity query, or use $this->entityManager->getStorage($entity_type_id)->getQuery()
Comment #14
JeroenT@tim.plunkett, hmm, I think it was done this way, because exists is a static method and $this is not available in it.
Comment #15
tim.plunkettYes, and I'm saying make it non-static, like the other exists() methods in forms.
Comment #16
JeroenTAlright, changed the method to a non-static method.
Patch attached.
Comment #18
tim.plunkettThanks!
Wrong class name
$this->entityQuery
Comment #19
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedFixed mentioned in #18
Not uploading any interdiff as it is just correction suggested by @tim.plunkett
Comment #20
tim.plunkettGreat, thanks!
Comment #21
alexpottBy definition this affect test only code and therefore is permitted in beta. Committed aecc5a4 and pushed to 8.0.x. Thanks!