Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Once #1615236: Merge entity controller interfaces, document and add default entity class definition is done, the field_test entity type will be working with classed objects. Still, it needs to be fixed to leverage the entity controller instead of coming up with its own CRUD functions. In addition to that, the entity type name violates the module namespace (entity type has to be prefixed with the module name) which should be fixed as well.
Comments
Comment #1
fagoadded tags
Comment #2
fagomoving to major, as those entities might be potentially causing issues / test-fails.
Comment #3
BerdirSuggestion: Move those example entity types to the entity test module.
They were probably started there because field tests needed them but there are now entity api tests in field.test as well, just because they rely on those entity types.
Comment #4
tim.plunkettDiscussed this with Berdir on IRC, this will conflict heavily with #1551140: Remove stub entities, replace entity_create_stub_entity() so postponing on that.
Comment #5
jstollerThis issue is now holding up #218755: Support revisions in different states.
Comment #6
BerdirIn case we want to merge it with entity_test, as suggested by me, we probably also want to wait on #1658712: Refactor test_entity schema to be multilingual. We can also do just what's minimally necessary to move on with the revision issue (what that other issue does is maybe already enough?) and postpone the merge?
Comment #7
BerdirThe entity stub issue has been commited, time to re-active this.
I'm working on this to get #1374030: Remove entity_extract_ids() now that we have proper classed entity objects working, code is currently there to make it easier to verify this, will extract it once it's working.
Comment #8
BerdirSee #1374030-38: Remove entity_extract_ids() now that we have proper classed entity objects, I'd prefer to postpone it on that issue, which does as much conversion/fixing as necessary to be able to remove entity_extract_ids(). My suggestions would be postpone this issue again on the other one and once commit get back to this and check if there's something remaining to fix.
I think the other issue should get us quite far and should also unblock other issues the revision stuff but I made some pragmatic choices that we might want to clean up.
Comment #9
BerdirYay, entity_extract_ids() is gone.
Let's see what's left here. I'd still like to basically move everything in field_test.entity.inc to entity_test.module, it's weird that we have two different test entity modules with different capabilities. But that might be a non-trivial task, with the overlapping feature set, ongoing multilingual property conversion and so on.
Comment #10
plachMarking as follow up of #1499596: Introduce a basic entity form controller: now we need also to introduce an
EntityTestFormController
. Agree with #3 that we should unify the field test entity and the regular test entity.Comment #11
sunBtw, could we do something about that, too? The identifiers we're currently using are anything but safe and reliable. They are not namespaced at all, so they essentially share the entire namespace violation and ambiguity problems of the hook system.
E.g.,
'taxonomy_term'
violates the namespace of a hypotheticaltaxonomy_term.module
. If the owner was properly namespaced, then the identifier would rather have to be'taxonomy.term'
. This particular example might be moot, but given stuff like ECK, I'm confident that we're going to run into major breakage at some point.(This could have an exception to the rule baked into the logic, saying that if the registered entity type equals the owner (
'node' == 'node'
), then the registered entity type is not prefixed with the owner namespace; i.e., retaining'node'
.)Additionally, we're collating all that info per module already, so it is a little weird that we're not automatically registering entity types by owner/namespace and thus remove the necessity of having to "prefix the entity type with the module name in entity info."
I realize that this is a bit off-topic here, but could not resist to mention it. If I'm making some sense to you guys, I'd love to see an issue for that.
Comment #12
BerdirOk, here is a first step.
Converting test_entity to the entity form controller. Was actually very easy. Not saving that much code because there isn't much to be saved, but it's a start.
Comment #13
BerdirFixed missing newline at the end.
Comment #15
BerdirHad to fix some manual drupal_get_form() calls in that test. Interesting, never seen that one before ;)
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commented#15: entity-form-controller-test-1616930-15.patch queued for re-testing.
Comment #17
aspilicious CreditAttribution: aspilicious commentedlooks good
Comment #18
BerdirNote: I think it makes sense to commit that patch as-is, but I still think we should merge entity_test and test_entity. That is currently however quite complicated, because test_entity is revisionable while entity_test has been converted to the new translatable properties style, so doing this would imply adding revision support for translatable properties in here, not sure if that makes sense.
The original concerns have been addressed (test_entity should not use custom CRUD functions), this will "fix" the form handling, maybe open a (non-major?) issue to merge test.entity.inc and entity_test.module?
Comment #19
fagoYeah, patch looks good! Thanks, Berdir!
While our current prefixing mechanism is not bullet-proof, I think it served us quite well in the past + it's what we keep doing else (hooks) as well. So I think we should better stay consistent here. For ECK, of course ECK entity types have to be prefixed by eck, what makes collisions unlikely.
I think it does, but the entity_test starts becoming very heavily used what makes it hard to change/improve it. Maybe we could do a second revisionable variant of it instead?
Also, #1696640: Implement API to unify entity properties and fields is reworking entity_test quite some, so I'd prefer to postpone any further entity_test changes for now.
Comment #20
Gábor HojtsyThis issue is still holding up #218755: Support revisions in different states FYI.
Comment #21
webchickI don't quite understand what relation these two issues could possibly have (nor why this is a major task), but this seems like a simple enough improvement.
Committed and pushed to 8.x. Thanks!