Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
4.36 KB

The attached patch basically does that except that I haven't moved the implementations yet, I'm forwarding the call to the storage controller.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, move-basefielddefinitions-2024963-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
71.71 KB

Re-roll and moved definitions around. Allows us to get rid of a storage controller or two, but most still have some methods in there. Noticed a problem in EntityTestStorageController, we can't remove that one because it needs access to the entity type in create(). We should pass that through to the static functions...

Status: Needs review » Needs work

The last submitted patch, move-basefielddefinitions-2024963-4.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
755 bytes
71.22 KB

An unrelated change ended up being in the patch.

Status: Needs review » Needs work

The last submitted patch, move-basefielddefinitions-2024963-6.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
497 bytes
71.15 KB

Removed the call to the new removed method on the storage controller.

Status: Needs review » Needs work

The last submitted patch, move-basefielddefinitions-2024963-8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
859 bytes
71.99 KB

This should be green.

chx’s picture

Status: Needs review » Reviewed & tested by the community

There is no doubt that baseFieldDefinitions have absolutely nothing to do with storage.

Berdir’s picture

Issue tags: +API change

This is a not yet approved API change.

This only impacts entities that already switched to EntityNG, of which I doubt there are many. Fetching the field definitions works just as it did before. Updating for this is easy, just move the method from the storage controller to the entity class and make it static.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Approved API change

This change makes total sense - approving.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -1236,4 +1236,11 @@ public function mergeDefaultDisplaysOptions() {
+  public static function baseFieldDefinitions($entity_type) {
+    return array();

Let's delegate this to the real View entity as ViewUI is a decorator used to add functionality when editing a View in the UI. Just in case config entities start to use this. How about something like return View::baseFieldDefinitions($entity_type);?

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.phpundefined
@@ -326,4 +326,19 @@ public function isTranslatable();
+   *
+   * @param string $entity_type
+   *   The name of the entity type to return properties from.

I think this should just be The entity type to return properties for. as this is consistent with other similar function params. I also think it might worth documenting the use case as it is never used in core AFAICS.

alexpott’s picture

Berdir mentioned on IRC that once #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface lands we will be able to remove the ViewUI code.

In that case I think we should only fix up the comment and outline the use-case for passing in $entity_type.

Berdir’s picture

Something like this?

chx’s picture

Status: Needs review » Reviewed & tested by the community

YES.

Berdir’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 48ddeb4 and pushed to 8.x. Thanks!

Berdir’s picture

Title: Move baseFieldDefinitions from storage to entity classes » Change notice: Move baseFieldDefinitions from storage to entity classes
Priority: Normal » Critical
Status: Fixed » Needs work
Issue tags: +Needs change record

Will provide a change notice around the whole field definition/base field definition stuff asap.

tstoeckler’s picture

Berdir’s picture

Title: Change notice: Move baseFieldDefinitions from storage to entity classes » Move baseFieldDefinitions from storage to entity classes
Priority: Critical » Normal
Status: Needs work » Fixed
Issue tags: -Needs change record

Added this to https://drupal.org/node/1806650, and more importantly, wrote a documentation page about all this at https://drupal.org/node/2078241. Reviews and improvements welcome!

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