Updated: Comment #0
Problem/Motivation
(Almost?) All config entities have a machine name form field with an 'exists' callback that checks for an existing entity of that type with a given name. Each config entity has to implement such a callback even though it can easily be done in a generic fashion. What's worse: Even though the best practice is to perform these 'exists' check with an entity query many config entities simply defer this to the 'foo_load()' function which performs completely unnecessary loading. This already happens several times in core and will be even more widespread in contrib, I fear.
Proposed resolution
It seems this is a great use-case for a generic exists() method on the form controller. This only concerns config entities, though, so we need to create a new ConfigEntityForm.
Remaining tasks
- Agree that this makes sense
- Write patch
- Review patch
- Commit patch
API changes
No BC-breaks. There's a new ConfigEntityFormController that it's a best practice to extend.
Comment | File | Size | Author |
---|---|---|---|
#71 | 2091871-nr-bot.txt | 144 bytes | needs-review-queue-bot |
#69 | 2091871-69.patch | 24.83 KB | Abhijith S |
#65 | 2091871-65.patch | 24.89 KB | sanjayk |
#61 | 2091871-61.patch | 26.04 KB | tvb |
#60 | interdiff_56_60.txt | 768 bytes | sanjayk |
Comments
Comment #1
BerdirThere are other things to consider for config entities. One is how they should handle config context, should they also enforce the context, like ConfigFormBase does?
If we need to do this, then it would be a very strong recommendation to use this as a base case.
Then there's the set/get topic, see #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface. And having one could rely on getExportProperties() to only set known properties on config entities, similar to what the content entity form controllers does.
Tagging to get Gabor in here, let's see if it works :)
Comment #2
tstoecklerUnless I'm mistaken #1 is an endorsement of this, right? :-)
Let's see how this works.
Comment #3
BerdirAn trailing space!!!111
Why EFQ and not just load it? EFQ on config entities is quite slow, because it has to load all of them to process them.
Unrelated changes?
Comment #5
tstoeckler1. PhpStorm--
2. It was said in other issues that this is a performance benefit over loading, as loading is conceptually unnecessary. I totally forgot that EFQ obviously needs to load for config entities. So I guess that should be changed.
3. Yes, sorry. That is a different patch I accidentally included.
Comment #6
jibran2: 2091871-config-form-controller-exists-2.patch queued for re-testing.
Comment #8
andypostRe-roll, suppose there could be different approach that used in core now
only
DateFormatFormBase
needs#field_prefix
EDIT
Only ShortcutSet is left
Comment #9
andypostComment #10
andypostAnother approach
Comment #13
andypoststatic can't be used in base classes, needs work
Comment #14
andypostFixed Imagestyle test, config tests are failed because
class ConfigQueryTest extends ConfigTest
Comment #17
BerdirThat might have been fixed by #2307681: getEntityTypeFromStaticClass - poor performance.
Comment #19
andypostno, that is not fixed... and it looks strange that it hard to inherit entity classes...
It looks that neither #8 no #10 works
Comment #20
andypostThis still needs work http://cgit.drupalcode.org/drupal/tree/core/modules/action/src/ActionFor...
Otoh this could be done latter
Comment #21
andypostAnother approach, the entity class could be overriden in contrib so forms shouldl use actual class name to load entity
Comment #22
andypostAs patch in #2465751: Remove config_test_load() and replace with entity_load shows there's still confusion which entity class to load
Comment #24
andypostFix image styles
Comment #26
andypostTrying the fix from related issue, seems better to implement the
exists()
methodComment #27
tim.plunkettHow is this an improvement? Seems lazier.
Same.
Etc...
Comment #28
andypostSo here's a more simplified version.
And I think better to add this method to EntityFormInterface so content entity forms could reuse that.
And this is a bug, because common forms use static
load()
method for this check.#2465751: Remove config_test_load() and replace with entity_load shows that this approach is not compatible when entities are extended with
hook_entity_type_alter()
and while core supports this better to use common method.Also @berdir mention about performance (for config entities)
Comment #29
andypostComment #30
tim.plunkettCan we try and do this in a BC compatible way please? Just add a ConfigEntityForm, don't touch EntityFormInterface.
Comment #31
andypostDo you mean add new
ConfigEntityForm
with public method but without interface?Comment #32
tim.plunkettYes. It's a callback, it only needs to be public because of how PHP works. We don't want code from other places calling it as though it were part of the interface.
Comment #33
andypostComment #34
OleksiyMade a re-roll of the patch #28. Added new
ConfigEntityForm
.Comment #35
andypostThanx, Oleksiy!
The only question for me a need in interface for the method
Comment #37
tim.plunkettThe last patch is missing the ConfigEntityForm.php file, so I can't review that :)
Please use
static::class
, not get_class()Every other hunk switches from load to an entity query, why is this doing the opposite?
Comment #38
OleksiyUps... Sorry for that. Have updated the patch according to your comments.
Comment #40
OleksiyUpdated the path to fix the failed tests. Reverted changes for the Shortcut. But
Migrate_drupal.Drupal\migrate_drupal\Tests\d6\EntityContentBaseTest
fails even on a clean Drupal installation.Comment #42
alexpottIt seems strange to swapping non static methods with injected dependencies for a static method using \Drupal. What's the rationale for this?
Comment #43
alexpottNot true... ::load() does this:
And ::getEntityTypeFromClass() takes into account alters... So any alters that use a new class will work.
Comment #51
andypostComment #52
Adev22 CreditAttribution: Adev22 at Google Code-In commentedrerolled the patch
Comment #54
jungle#52 is totally wrong
Comment #55
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #56
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedReroll patch #40.
Comment #57
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedPlease ignore #56.
Comment #59
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #60
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented#57 patch applied successfully and test cases passed. Fixed coding standard issues as well.
Comment #61
tvb CreditAttribution: tvb commentedPatch from #60 could not be applied.
Rerolled patch attached.
Additionnaly removed excess whitespace in DateFormatFormBase.php.
Comment #62
tvb CreditAttribution: tvb commentedComment #64
tanubansal CreditAttribution: tanubansal at Salsa Digital commentedTested #61 on 9.1, exists() method has been added and white space has also been removed
Comment #65
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRe-roll the patch for 9.2
Comment #69
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedRerolled patch #65 for 9.5.x.
Comment #71
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.