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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Issue tags: +Entity system, +sprint

added tags

fago’s picture

Priority: Normal » Major

moving to major, as those entities might be potentially causing issues / test-fails.

Berdir’s picture

Suggestion: 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.

tim.plunkett’s picture

Status: Active » Postponed

Discussed this with Berdir on IRC, this will conflict heavily with #1551140: Remove stub entities, replace entity_create_stub_entity() so postponing on that.

jstoller’s picture

Berdir’s picture

In 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?

Berdir’s picture

Assigned: Unassigned » Berdir
Status: Postponed » Active

The 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.

Berdir’s picture

Assigned: Berdir » Unassigned
Status: Active » Postponed

See #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.

Berdir’s picture

Status: Postponed » Active

Yay, 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.

plach’s picture

Title: fix the field_test entity type » Fix the field_test entity type
Issue tags: +Entity forms

Marking 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.

sun’s picture

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.

Btw, 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 hypothetical taxonomy_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.

Berdir’s picture

Status: Active » Needs review
FileSize
6.02 KB

Ok, 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.

Berdir’s picture

Fixed missing newline at the end.

Status: Needs review » Needs work

The last submitted patch, entity-form-controller-test-1616930-13.patch, failed testing.

Berdir’s picture

Had to fix some manual drupal_get_form() calls in that test. Interesting, never seen that one before ;)

moshe weitzman’s picture

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

looks good

Berdir’s picture

Note: 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?

fago’s picture

Yeah, patch looks good! Thanks, Berdir!

E.g., 'taxonomy_term' violates the namespace of a hypothetical taxonomy_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.

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.

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.

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.

Gábor Hojtsy’s picture

This issue is still holding up #218755: Support revisions in different states FYI.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I 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!

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