Updated: Comment #10

Problem/Motivation

To help general DX/code organisation bring entity bundle logic into the EntityManager. This will benefit unit testing (mocking etc..) and remove the need for calling a procedural function.

Proposed resolution

Convert entity_get_bundles() function in entity.inc to methods on the EntityManager. Add getBundleInfo() and getAllBundleInfo(), the module handler is already injected into EntityManager so we can use that.

Remaining tasks

None, patch ready.

User interface changes

None

API changes

Deprecate entity_get_bundles() in favour of new methods?

Original report by @dawehner

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
3.71 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, drupal-2030151-1.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
811.16 KB

The bundle labels were still using $bundles variable, and we need to clear the bundleInfo static cache when the entity info definitions are cleared.

damiankloip’s picture

FileSize
4.27 KB

Tim kindly pointed out that I had accidentally included the whole world in the last patch. interdiff is still good!

Status: Needs review » Needs work

The last submitted patch, 2030151-4.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.28 KB

ok, lets try once more. Interdiff in #3 is still correct.

Status: Needs review » Needs work

The last submitted patch, 2030151-6.patch, failed testing.

damiankloip’s picture

Forgot about this issue, rerolled and fixed the remaining failures in EntityReferenceAdminTest. Not sure why the current tests failed without this fix though... Using key() to get the bundle after iterating over it will return NULL as there is no current key to return.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update

All looks fine beside the missing issue summary.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Thank you

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/entity.inc
@@ -58,33 +57,12 @@ function entity_info_cache_clear() {
  *   The bundle info for a specific entity type, or all entity types.
  */
 function entity_get_bundles($entity_type = NULL) {

I think we need at least an @see here to the new getBundleInfo and getAllBundleInfo methods.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
519 bytes
5.15 KB

There we go.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The feedback from alex got adressed, so back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ea8a4bd and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Added proper issue summary

YesCT’s picture

Issue summary: View changes

Shall we have a follow-up to actually replace entity_get_bundles() that already exist in core?

Mile23’s picture

Shall we have a follow-up to actually replace entity_get_bundles() that already exist in core?

Yes. :-) #2427637: Remove usages of deprecated entity_get_bundles()

Mile23’s picture