Now as the new entity field API got committed, we need to convert existing entity types to make use of it. See #1346214: [meta] Unified Entity Field API and the "Entity Field API" tag.

We decided to exploit the EntityBCDecorator to perform a partial conversion, allowing us to convert only the key pieces of code an leave the rest to follow-ups. This in turn leads to more manageable patches and less changes in a single issue. To be able to follow this approach we need to change all type hints referring to the Node entity to just EntityInterface, which moreover is the recommended way according to our coding standards. This will also pave the way for #1810370: Entity Translation API improvements.

Follow-ups

CommentFileSizeAuthor
#196 interdiff-node-ng-1818556-184-196.txt577 bytesdas-peter
#196 node-ng-1818556-196.patch199.7 KBdas-peter
#184 interdiff-node-ng-1818556-182-184.txt948 bytesdas-peter
#184 node-ng-1818556-184.patch199.76 KBdas-peter
#182 interdiff-node-ng-1818556-178-182.txt5.29 KBdas-peter
#182 node-ng-1818556-182.patch200.16 KBdas-peter
#178 interdiff-node-ng-1818556-176-178.txt5.34 KBdas-peter
#178 node-ng-1818556-178.patch202.4 KBdas-peter
#176 interdiff-node-ng-1818556-174-176.txt1.08 KBdas-peter
#176 node-ng-1818556-176.patch200.94 KBdas-peter
#174 interdiff-node-ng-1818556-169-174.txt5.26 KBdas-peter
#174 node-ng-1818556-174.patch200.67 KBdas-peter
#170 node-ng-1818556-169.patch199.24 KBdas-peter
#170 interdiff-node-ng-1818556-168-169.txt815 bytesdas-peter
#168 interdiff-node-ng-1818556-166-168.txt1.35 KBdas-peter
#168 node-ng-1818556-168.patch199.22 KBdas-peter
#166 interdiff-node-ng-1818556-161-166.txt5.01 KBdas-peter
#166 node-ng-1818556-166.patch199.57 KBdas-peter
#161 interdiff-node-ng-1818556-159-161.txt1.37 KBdas-peter
#161 node-ng-1818556-161.patch198.43 KBdas-peter
#159 node-ng-1818556-159.patch197.7 KBdas-peter
#152 node-ng-1818556-152.patch197.69 KBdas-peter
#152 interdiff-node-ng-1818556-148-152.txt3.3 KBdas-peter
#151 POC-node-ng-1818556-151-POC.patch198.15 KBdas-peter
#148 interdiff-node-ng-1818556-147-148.txt995 bytesdas-peter
#148 node-ng-1818556-148.patch198.38 KBdas-peter
#147 node-ng-1818556-147.interdiff.do_not_test.patch760 bytesplach
#147 node-ng-1818556-147.patch198.58 KBplach
#143 node-ng-1818556-143.patch.interdiff.do_not_test.patch5.29 KBplach
#143 node-ng-1818556-143.patch.patch197.83 KBplach
#142 1818556-node-ng-142.patch197.3 KBvijaycs85
#142 1818556-diff-135-142.txt4.96 KBvijaycs85
#135 node-ng-1818556-135.interdiff.do_not_test.patch4.82 KBplach
#135 node-ng-1818556-135.patch194.3 KBplach
#132 interdiff-node-ng-1818556-120-132.txt6.85 KBdas-peter
#132 node-ng-1818556-132.patch189.78 KBdas-peter
#120 interdiff-node-ng-1818556-118-120.txt3.18 KBdas-peter
#120 node-ng-1818556-120.patch188.65 KBdas-peter
#118 node-ng-1818556-118.patch185.95 KBdas-peter
#116 interdiff-node-ng-1818556-115-116.txt783 bytesdas-peter
#116 node-ng-1818556-116.patch185.9 KBdas-peter
#115 node-ng-1818556-115.patch185.78 KBdas-peter
#115 interdiff-node-ng-1818556-112-115.txt466 bytesdas-peter
#112 interdiff-node-ng-1818556-106-112.txt3.76 KBdas-peter
#112 node-ng-1818556-112.patch185.76 KBdas-peter
#106 interdiff-node-ng-1818556-104-106.txt4.79 KBdas-peter
#106 node-ng-1818556-106.patch193.68 KBdas-peter
#104 interdiff-node-ng-1818556-97-104.txt3.51 KBdas-peter
#104 node-ng-1818556-104.patch189.73 KBdas-peter
#97 interdiff-node-ng-1818556-95-97.txt10.22 KBdas-peter
#97 node-ng-1818556-97.patch178.92 KBdas-peter
#95 interdiff-node-ng-1818556-92-95.txt4.3 KBdas-peter
#95 node-ng-1818556-95.patch168.88 KBdas-peter
#92 interdiff-node-ng-1818556-90-92.txt3.41 KBdas-peter
#92 node-ng-1818556-92.patch166.41 KBdas-peter
#90 interdiff-node-ng-1818556-89-90.txt3.59 KBdas-peter
#90 node-ng-1818556-90.patch163.56 KBdas-peter
#89 interdiff-node-ng-1818556-87-89.txt1.11 KBdas-peter
#89 node-ng-1818556-89.patch161.15 KBdas-peter
#87 interdiff-node-ng-1818556-85-87.txt780 bytesdas-peter
#87 node-ng-1818556-87.patch160.43 KBdas-peter
#85 node-ng-1818556-85.patch159.67 KBdas-peter
#85 interdiff-node-ng-1818556-81-85.diff8.11 KBdas-peter
#81 interdiff-node-ng-1818556-79-81.txt3.69 KBdas-peter
#81 node-ng-1818556-81.patch157.92 KBdas-peter
#79 node-ng-1818556-79.patch155.46 KBdas-peter
#79 interdiff-node-ng-1818556-76-79.txt4.26 KBdas-peter
#76 node-ng-1818556-76.patch154.59 KBdas-peter
#76 interdiff-node-ng-1818556-70-76.diff2.75 KBdas-peter
#71 node-ng-1818556-70.diff152.69 KBdas-peter
#71 interdiff-node-ng-1818556-62-70.txt9.29 KBdas-peter
#69 ng_fix_autocreate.patch.txt4.48 KBfago
#68 d8_piggy_back.patch2.15 KBfago
#63 node-ng-1818556-62.patch147.1 KBdas-peter
#63 interdiff-node-ng-1818556-55-62.txt28.02 KBdas-peter
#55 node-ng-1818556-55.patch146.37 KBplach
#54 node-ng-1818556-54.interdiff.do_not_test.patch6.72 KBplach
#51 node-ng-1818556-51.patch122.22 KBplach
#50 node-ng-1818556-49.interdiff.do_not_test.patch7.36 KBplach
#49 node-ng-1818556-49.patch119.17 KBplach
#47 node-ng-1818556-47a.patch122.52 KBplach
#47 node-ng-1818556-47b.patch123.15 KBplach
#45 node-ng-1818556-45.patch121.08 KBplach
#43 node-ng-1818556-43.patch117.24 KBplach
#40 node-ng-1818556-40.patch116.07 KBplach
#38 node-ng-1818556-38.patch112.38 KBplach
#34 node-ng-1818556-34.patch112.29 KBplach
#31 node-ng-1818556-31.patch112.33 KBplach
#22 d8_nodes.patch32.5 KBfago
#17 node-ng-1818556-16.do_not_test.patch31.64 KBBerdir
#15 node-ng-1818556-15.do_not_test.patch16.38 KBplach
#6 entity_compatibility.patch8.09 KBfago
#1 drupal-node-EntityNG-1818556-1.diff20.61 KBdas-peter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

das-peter’s picture

Status: Active » Needs review
FileSize
20.61 KB

As mentioned here #1498674-83: Refactor node properties to multilingual I've started to build an adapter with the goal to make the NG things fully backwards-compatible.
As I'm not able to continue the work over the weekend I'd like to post what I've done so far.

To use an entity class with EntityNG fully backward compatible following steps are necessary:

  1. Extend EntityNGHelper instead Entity in the entity class. EntityNGHelper is the decorator which ensures full backward compatibility.
  2. Extend DatabaseStorageControllerNG in the entity storage controller.
  3. Add baseFieldDefinitions() to the entity storage controller to declare the entity properties.
  4. Extend EntityFormControllerNG in the entity form controller.
  5. Ensure that places where entity objects are cast to array are changed to this pattern $entity->__toArray().

Known issues:

  • The revision handling as it is no available in DatabaseStorageController is missing in DatabaseStorageControllerNG. I tried to adapt it but it's not complete yet.

EntityNGHelper tries to be a smart-ass and unsets the entity properties in the init() automatically. I'm not sure if this approach works in all cases, maybe we should require to implement init() even if EntityNGHelper is used - similar to the creation of baseFieldDefinitions() which's necessary in any scenario.

The patch contains parts from here: #1778178: Convert comments to the new Entity Field API

Let's see what the testbot says, fails expected. My local tests fail primarily on revision related stuff.

Status: Needs review » Needs work

The last submitted patch, drupal-node-EntityNG-1818556-1.diff, failed testing.

fago’s picture

While discussing on IRC with das-peter we came to the following idea for easing conversion:

  • For not fully converting an entity type over (so e.g. $node->title keeps working) we force compatibility mode to stay enabled per entity type.
  • Then, we implement $entity->ng() which returns a decorator allowing us to work with the new Entity Field API as usual, i.e. doing $entity->ng()->title->value; (i.e. forward __get() calls to $this->entity->get($field))
  • For fully converted entitty types we also implement $entity->ng() but just return $this there. Then all the NGcontrollers like the form, storage and render controller can go with $entity->ng()->field and/or just add the line $entity = $entity->ng(); to the beginning of their methods.

This should help use to speed up conversions as existing code continues work. At the same time new code can start leveraging new functionality via $entity->ng(). This workaround is not nice, but I think it's the most practical at the moment. Once the conversion is fully done, all the ng() calls should be removed rather easily by some search/replace magic.

plach’s picture

Assigned: Unassigned » plach

Working on this.

fago’s picture

Assigned: plach » fago

Taking over for now.

fago’s picture

Status: Needs work » Needs review
FileSize
8.09 KB

ok, here is patch implementing the compatibility mode helper as discussed. Then, when we implement the Node class based upon EntityNG we can force it to have compatibility mode enabled.

All the controllers around like the form controller can probably stay with the non-NG variant for now then.

plach’s picture

Assigned: fago » plach

Taking back :)

plach’s picture

Issue tags: +D8MI, +language-content

tagging

das-peter’s picture

I've created this spin-off: #1831444: EntityNG: Support for revisions for entity save and delete operations
Hope this doesn't interfere with the work of plach.

das-peter’s picture

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityCompatibilityDecorator.phpundefined
@@ -0,0 +1,254 @@
+  /**
+   * Forwards the call to the decorated entity.
+   */
+  public function isNewRevision() {
+    return $this->decorated->isNewRevision();

The other decorators in core all use __call() to pass all calls through. And when implementing an interface, the proper pattern is


  /**  
   * Implements \IteratorAggregate::getIterator().
   */
  public function getIterator() {
    return $this->__call(__FUNCTION__, func_get_args());
  }

The docblock needs to indicate the exact method, and each body can use the same __call()


  /**  
   * Passes through all unknown calls onto the decorated object.
   */
  public function __call($method, $args) {
    return call_user_func_array(array($this->decorated, $method), $args);
  }

YesCT’s picture

I think this is still blocking #1498674: Refactor node properties to multilingual
might be waiting to come back to this after one of the other issues though

fago’s picture

Status: Needs work » Needs review

as discussed, I've created a patch which enable the BC decorator to work with converted entity properties - see #1890242: Improve the EntityBCDecorator to support converted entity properties.. This would enable use to go with two-step conversion as described at #1818580: [Meta] Convert all entity types to the new Entity Field API.

plach’s picture

I'll start working on this tonight. I'll incorporate #1890242: Improve the EntityBCDecorator to support converted entity properties. as part of this issue to be able to proceed. We will just merge it in after it's committed.

plach’s picture

Here is a first stab: it doesn't work at all, but wanted to post it here anyway. At the moment I'm stuck with a fatal in the BC decorator: Comment tries to access $node->comment and fails because the $node->decorated->values array holds the 'comment' value in $node->decorated->values[LANGUAGE_DEFAULT] instead of $node->decorated->values[LANGUAGE_DEFAULT][0]['value']. Not sure why, but it seems I cannot make it behave as in EntityFieldTest::testBCDecorator().

I'll go on with this tomorrow.

plach’s picture

Issue tags: +sprint
Berdir’s picture

Issue tags: -sprint
FileSize
31.64 KB

Played around with it a bit too. I was able to hack around until the example test case there worked but it's ugly.

- Also passing the BC entity to hooks, return it from create and pass it to a number of node specific functions.

- Type hints are tricky: I had to copy EntityBCDecorator to NodeBCDecorator and extend that from Node:

Argument 1 passed to comment_node_insert() must be an instance of Drupal\node\Plugin\Core\Entity\Node, instance of Drupal\Core\Entity\EntityBCDecorator given

- Not sure what's going on with the decorator exactly but at some point, the values are directly in $this->decorated->values['und'], without any subkeys. Maybe that's an optimization from fago somehow? Or related to the fact that we have no data table?

- Loading by properties seems to be broken, or the values are not properly stored, haven't checked.

- $node->original is also a fun case, added a special case for that.

The Node Save test now runs through without fatal error or exception but there are still a few exceptions and failures.

chx’s picture

Issue tags: +sprint

crosspost.

webchick’s picture

Priority: Normal » Major

This is at least major, if not critical, because translations are basically useless without node titles being translatable, which I understand requires this to happen.

plach’s picture

Assigned: plach » Unassigned
Priority: Major » Normal

Merged #16 in http://drupalcode.org/sandbox/plach/1719670.git/tree/refs/heads/8.x-et-n.... Couldn't work on this more, sorry.

I'll reassign this back to me as soon as I can work on it again (hopefully tomorrow).

plach’s picture

Priority: Normal » Major
fago’s picture

FileSize
32.5 KB

ok, I gave this a short test. I was able to get it working in principle, such that I received a close-to working node-view page. However, I must say all that bc-decorator wrappers being passed around and sometimes not makes it rather complex. I fear it requires quite some work to get all tests green - lots of work that eats our time we could use to the real conversion instead - which is lots of work but not difficult to do. Thus my feeling is we should better go for the straight conversion and work together to move on fast. Thoughts?

@Git: Let's use a single sandbox for collaboration so we can easily merge each others changes? I don't really care which one, but we could continue using the entity-api sandbox and branches: http://drupal.org/sandbox/fago/1497344 - you all should already have access?

I pushed field-node (based on plach's sandbox), field-node-base (as 8.x base for diffing) and a field-node-fago branch to the repo. (Always *merge* changes, never re-base)

Also attaching the patch. It makes node_load() return BCEntities while entity_load() stays with EntityNG.

- Type hints are tricky: I had to copy EntityBCDecorator to NodeBCDecorator and extend that from Node:

We can avoid this by moving the decorator to the Node class, while the real entity becomes NodeNG. See attached patch.

Status: Needs review » Needs work

The last submitted patch, d8_nodes.patch, failed testing.

plach’s picture

OK, it seems it will be easier to do this if we get these in first...

Berdir’s picture

To complete it maybe. But I don't think there is anything that prevents us from working on both things in parallel. My suggestion would be to simply not touch any field related tests yet, there is a lot of code to convert everywhere else in core.

fago’s picture

But problem is that this patch will become rather big, so keeping it updated and fixing merge conflicts will be a significant amount - it was already for comments, for nodes I'd think it's way worse. Thus, once we get started with the full conversion we should try to get it done+in asap.

fago’s picture

I recalled I promised to post my regex, so here is the regex I used for comments (with PHPstorm):

\$comment([_a-z0-9]*)?->([a-z_]+)(\[LANGUAGE_NOT_SPECIFIED\]\[0\]\['value'])?(->value)?
\\$comment$1->$2->value

It's not perfect, but a big help ;-)

plach’s picture

I got back to this yesterday night: since in #1810370: Entity Translation API improvements we decided to introduce another decorator to deal with entity translation language, I decided to go back to the original strategy and try to pass the BC decorator around by changing type hints. I'm making some progress, but I have not a new patch ready, I'll try to post one tonight.

moshe weitzman’s picture

Thanks for reviving this, plach. This is a pretty major task for the Web Services effort.

moshe weitzman’s picture

Issue tags: +WSCCI
plach’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI
FileSize
112.33 KB

Ok, I was able to complete a full UI-driven CRUD workflow. Let's see how many test failures we have.

plach’s picture

Issue tags: +WSCCI

Oops

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-31.patch, failed testing.

plach’s picture

FileSize
112.29 KB

Fixed Forum syntax.

plach’s picture

Status: Needs work » Needs review
YesCT’s picture

#31: node-ng-1818556-31.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +D8MI, +sprint, +language-content, +Entity Field API

The last submitted patch, node-ng-1818556-34.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
112.38 KB

This one should have less failures.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-38.patch, failed testing.

plach’s picture

FileSize
116.07 KB

More fixes.

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-40.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
117.24 KB

More :)

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-43.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
121.08 KB

And again!

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-45.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
123.15 KB
122.52 KB

Let's try two different approaches.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-47b.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
119.17 KB

This one reverts some optimizations to the BC decorator and goes back closer to #38. Not sure what to think about the increasing number of failures.

plach’s picture

Forgot the interdiff: @fago do you think there's something wrong in here that could cause troubles?

plach’s picture

FileSize
122.22 KB

Another fix.

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -46,13 +46,43 @@ class EntityBCDecorator implements IteratorAggregate, EntityInterface {
+  function __construct(EntityNG $decorated, array &$definitions, array &$values, array &$fields) {
     $this->decorated = $decorated;
+    $this->definitions = &$definitions;
+    $this->values = &$values;

I tried that already - it will create troubles with cloning the values when the entity is cloned - check the entity uuid tests. There is a comment somerwhere noting this.

Thus, I'd suggest to not re-work the BC-decorator that way - I already spent too much (lost) time on that.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -367,7 +367,8 @@ public function translations() {
-      $this->bcEntity = new EntityBCDecorator($this);
+      $this->getPropertyDefinitions();
+      $this->bcEntity = new EntityBCDecorator($this, $this->fieldDefinitions, $this->values, $this->fields);

This should just pass through the return of the method if it has to use it already.

plach’s picture

Ok, I'll revert that part.

plach’s picture

@fago:

Here is my current BC decorator. I retained definitions cache to speed things up a bit. I don't think this should be a problem, right?

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -53,36 +53,16 @@ class EntityBCDecorator implements IteratorAggregate, EntityInterface {
+    $this->definitions = $definitions;

The interdiff is a bit old. I actually restored:

$this->definitions = &$definitions;
plach’s picture

FileSize
146.37 KB

A big bunch of fixes.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-55.patch, failed testing.

YesCT’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -46,13 +46,23 @@ class EntityBCDecorator implements IteratorAggregate, EntityInterface {
    * @param \Drupal\Core\Entity\EntityInterface $decorated
...
+  function __construct(EntityNG $decorated, array &$definitions) {

the type does not appear to match.

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -119,20 +148,29 @@ public function &__get($name) {
+        // If field API sets a value with a langcode in entity language, move it
+        // to LANGUAGE_DEFAULT.
+        // This is necessary as EntityNG does key values in default language always
+        // with LANGUAGE_DEFAULT while field API expects them to be keyed by
+        // langcode.

check these for 80 chars.

I got interrupted looking at this. It's probably too early for me to look at it anyway. :)

fago’s picture

@plach: Passing definitions by reference should work as long as a cloned entity does not change bundles. It's not optimal, but should work out temporarily.

plach’s picture

Yep, that's what I thought.

jibran’s picture

Priority: Major » Critical

#1498674: Refactor node properties to multilingual is critical and postponed on this.

das-peter’s picture

I think quite some fails are provoked by the EntityFormControllerNG the method buildEntity() relies on the information provided by $translation->getPropertyDefinitions();:

    $translation = $entity->getTranslation($this->getFormLangcode($form_state), FALSE);
    $definitions = $translation->getPropertyDefinitions();
    foreach ($values_excluding_fields as $key => $value) {
      if (isset($definitions[$key])) {
        $translation->$key = $value;
      }
    }

Thus if there are no definitions e.g. for the book property used by the book module (has a book group on the node form) these form values are lost.
If I look at entity_form_submit_build_entity() following could work as a very sloppy workaround:

    foreach ($values_excluding_fields as $key => $value) {
      if (isset($definitions[$key]) || $entity instanceof EntityBCDecorator) {
        $translation->$key = $value;
      }
    }
plach’s picture

Yes, that was the next item on my list :)

das-peter’s picture

Status: Needs work » Needs review
FileSize
28.02 KB
147.1 KB

Just out of curiosity here a patch with the mentioned change. The interdiff is quite big because I've merged the latest 8.x.
I'm wondering how the test-fails are affected by this little change.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-62.patch, failed testing.

plach’s picture

Brilliant!

das-peter’s picture

Status: Needs work » Needs review

While continue the work I struggled with this a test failure in the test Node access on any table
TaxonomyTermReferenceItem::setValue() has a quite picky test for valid values:

    if ($values) {
      throw new \InvalidArgumentException('Property ' . key($values) . ' is unknown.');
    }

It seems like it can happen that the values (name, vid) from TaxonomyAutocompleteWidget::massageFormValues() get stuck, and thus are preset in e.g. EntityFormControllerNG::buildEntity().
Calling there $entity->getTranslation($this->getFormLangcode($form_state), FALSE) drills down to TaxonomyTermReferenceItem::setValue(), but there was not even a chance to clean up the raw form values which then throws the error.
I guess this could be avoided if there was another way than initializing allllll promperties to figure out which translation languages are available.
For now I've just shut up the picke values test by adding name, vid to the cleanup process in TaxonomyTermReferenceItem::setValue().
Addition: InvalidArgumentException is still thrown - but now because a strange array is provied as $value to TaxonomyTermReferenceItem::setValue() - it looks like a term entity converted to an array. And I've no clue where it comes from and why the heck it is set.

I try to track that down but unfortunately the format of the passed array throws an internal exception in phpStorm when debugging the code...
Any ideas / hints very welcome :)

fago’s picture

ok, problem is field API piggy-backing values in $item during auto-creation.

I tried fixing it properly by having the new entity in $item['entity'] see attached patch. But it won't work out as a) in bc mode the entity won't be accessible by default and b) it looks like rendering also relies on $item['taxonomy_term'].

So we probably should do an interim fix by declaring the added stuff for the taxonomy reference item also. OR we bet on #1847596: Remove Taxonomy term reference field in favor of Entity reference to go in first and it there. The same problem happens to be there for the entity reference item.

fago’s picture

FileSize
2.15 KB

ok, I had another idea: We could just allow piggy-backing any data on $items for now. This should help us moving on with the conversion faster, we can remove/refactor it once conversion is complete. Then we actually can rely on our new API to be there and use it...

Attached is a totally untested patch which only works if I did not miss anything. But at least it should show the idea. The BC-decorator uses getValue() so it should pick the extra stuff up by having it there.

fago’s picture

FileSize
4.48 KB

oh and the missing patch from #67 showing the idea on how to properly deal with to-be created entities

Status: Needs review » Needs work

The last submitted patch, d8_piggy_back.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
9.29 KB
152.69 KB

@fago: Thank you very much for the patches. I just had to adjust some minor things. Now the Node access on any table tests pass :)

The new Patch contains additions of fago and some other adjustments. Some more tests should pass now.
Latest changes are pushed into fagos sandbox (http://drupalcode.org/sandbox/fago/1497344.git/shortlog/refs/heads/field...)

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-70.diff, failed testing.

sun’s picture

Any chance for doing all of those type-hint/phpDoc changes in a separate issue?

tim.plunkett’s picture

+++ b/core/modules/translation/tests/translation_test.moduleundefined
@@ -5,11 +5,11 @@
-use Drupal\node\Plugin\Core\Entity\Node;
+use Drupal\Core\Entity\EntityInterface;
...
-function translation_test_node_insert(Node $node) {
+function translation_test_node_insert(EntityInterface $node) {

Agreed, changes like these are completely out of scope.

das-peter’s picture

As far as I know not all of these changes are just cosmetic but necessary as we deal with a decorated entity. And as EntityBCDecorator is a wrapper that implements EntityInterface and doesn't extend some specific entity class using the EntityInterface seems to me the only viable way to do the type hinting.

das-peter’s picture

Assigned: Unassigned » das-peter
Status: Needs work » Needs review
FileSize
2.75 KB
154.59 KB
  • Test: Node access
    Fixed conditions
  • Test: Node access and fields
    Defined default values weren't assigned in field_populate_default_values() because EntityBCDecorator::__get() initializes the fields data array with NULL values. Thus the check in EntityBCDecorator::__isset() always returned TRUE. I've changed the check in EntityBCDecorator::__isset() to ensure there is a non-NULL value in the data array. It's horrible code but the only solution I can think of - as far as I remember initializing the values was essential and thus I didn't want to change there anything.
    Other ideas very welcome!

And as seen in the interdiff I made a Node to EntityInterface just in a function doc block. Not sure if really necessary, but I personally prefer to have consistency between function declaration and its doc block over a smaller diff.

Status: Needs review » Needs work

The last submitted patch, interdiff-node-ng-1818556-70-76.diff, failed testing.

das-peter’s picture

The test-Failure in Node creation is related to this #1872690: Exception: Serialization of 'Closure' is not allowed in serialize. Dealing with the expected exception leads to the php error Serialization of 'Closure' is not allowed.
I think this should be handled in the related issue.

das-peter’s picture

Status: Needs work » Needs review
FileSize
4.26 KB
155.46 KB

Test: Node save
Handling of the properties enforceIsNew, newRevision and isDefaultRevision moved into EntityNG since these properties are part of Entity. I don't see why these properties should be handled by Node itself. Other opinions?
Fixed handling of the original property in EntityBCDecorator::__get().

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-79.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
157.92 KB
3.69 KB

Test: Node revisions all
Added handling of the log property.
Changed the way how NodeRevisionsAllTestCase::setUp() creates node revisions (using get_object_vars() isn't viable anymore).
Added methods isNewRevision() and isDefaultRevision() to EntityNG (use ->value to access value).

Hmm, looks like the overall amount of test-failures increased horribly :|

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-81.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -530,4 +530,23 @@ public function label($langcode = NULL) {
+  public function isNewRevision() {
+    $info = $this->entityInfo();
+    return $this->newRevision->value || (!empty($info['entity_keys']['revision']) && !$this->getRevisionId());
+  }
+
+  /**
+   * Overrides Entity::isDefaultRevision().
+   */
+  public function isDefaultRevision($new_value = NULL) {
+    $return = $this->isDefaultRevision->value;
+    if (isset($new_value)) {
+      $this->isDefaultRevision = (bool) $new_value;
+    }
+    return $return;

That doesn't make sense?

Both are just normal, protected class properties which are not persisted and only accessed through methods. There is no reason to define them as entity fields?

And if it would be, then the isDefaultRevision assignment would be wrong.

das-peter’s picture

Status: Needs work » Needs review

@Berdir You're absolutely right. Atm. I'm fighting with getting the revisions stuff done. The changes worked for the decorated node entity but not for all other NG entities (e.g. comments). However, if I revert the stuff I can't manage to get the test Node revisions all passing.
Unfortunately I wasn't able yet to figure out what's going wrong and where :|

das-peter’s picture

I think I found the issue:
EntityNG::__construct() doesn't set values for the fixed class properties.
DatabaseStorageControllerNG::mapFromStorageRecords() prepares all values to be used for defined fields, thus EntityNG::__construct() misses them.
I've added a check if a value belongs to an existing class property and if it does the value is set to the class property. This shouldn't harm the properties for which a definition exists and are class properties at the same time - these will be reset with the later call to EntityNG::init().

The interdiff contains several changes I'm not so sure about:

  • EntityBCDecorator::__get(): Special handling of the "magic" flag revision.
  • NodeSaveTest::testImport(): Explicit usage of the method Entity::enforceIsNew() instead using a value in the node data array. Has there to be support for the enforceIsNew flag in the data array?
    The problem with the flag in the data array is that DatabaseStorageControllerNG::create() can't access the protected entity class properties. A solution could be to check if there's a method with the name of the data array item and call it with the value as parameter (would work for enforceIsNew). But I don't like to introduce even more magic ;)

Status: Needs review » Needs work

The last submitted patch, interdiff-node-ng-1818556-81-85.diff, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
160.43 KB
780 bytes

Very small change in FileFieldTestBase to fix the creation of test nodes. Should affect quite some other tests.
I hope it reduces the failures even more, but there's the risk that the field tests now run as the should and fail somewhere else ;)

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-87.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
161.15 KB
1.11 KB

Another tiny change to fix an issue that nearly drove me nuts ;)
We need to check if a field is not empty before pre-setting its data-structure to the values in EntityBCDecorator::__get().
Otherwise we can end up with ghost values like:

$this->decorated['values']['field_tags']['und'][0] = array(
  'tid' => NULL,
  'entity' => NULL,
);

Stuff like this can cause strange errors - e.g. array_flip(): Can only flip STRING and INTEGER values in DatabaseStorageController::load(). This function tries to flip the passed id's and has trouble when one of them is NULL.

das-peter’s picture

Some minor changes:

  • DatabaseStorageControllerNG::__construct() tests now if the bundle key is set before accessing it in the passed in array.
  • NodeStorageController defines now the data type string_field instead text_field to only rely on core data types. Is there a string length limit in that type?
  • PluginTestBase & MockBlockManager use now Drupal\Core\Entity\EntityBCDecorator instead Drupal\node\Plugin\Core\Entity\Node as node class.
  • DefaultViewsTest now properly creates a node type before testing. (Otherwise the body field is missing and threat as property)

With this changes some more tests should pass :)

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-90.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
166.41 KB
3.41 KB
  • Test: Entity Reference UI
    field_ui_field_edit_form() calls _field_create_entity_from_ids() which triggers creating an entity object.
    Unfortunately creating an entity triggers EntityNG::getPropertyDefinition() which ends up in TypedDataManager::create(), where the method tries to deal with the not yet created field. This fails because the definition of the not created yet field misses the 'type' in $definition.
    I don't think this is an EntityBCDecorator but an EntityNG specific issue.
    Ideas how we can fix this?
  • Test Entity Translation:
    Had to map 'value' to 'target_id' in EntityReferenceItem::__set() and EntityReferenceItem::__get(). There was already a mapping in EntityReferenceItem::get() but that wasn't sufficient.
  • Test Taxonomy term functions and forms
    In taxonomy.module 'taxonomy_term' instead 'entity' was used - causing TaxonomyTermReferenceItem to throw an exception.
    Another occurence of array_flip(): Can only flip STRING and INTEGER values caused by node_preview() - where the terms are expected to be listed, even if not yet created. Solved it by adding an other check to not only skip 'autocreate' but also FALSE tids in taxonomy_field_formatter_prepare_view().
Berdir’s picture

field_ui_field_edit_form() calls _field_create_entity_from_ids() which
triggers creating an entity object.

Uh, that sounds strange. That *very very ugly* helper function was added to be able to create a fake entity object when deleting fields as the entity might not exist anymore at that point. Will look into it.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-92.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
168.88 KB
4.3 KB
  • Test File field widget test
    More trouble with EntityBCDecorator::__isset() an empty array has to be considered as set, even if an array with only NULL values has to be considered as not set.
    Otherwise a cleared field isn't stored back and its data persist.
  • Test Forum blocks
    Since LANGUAGE_DEFAULT is mapped to an actual language we've to handle the languages different in forum_field_storage_pre_insert() to avoid inserting duplicates into forum_index. Not sure if the approach I've chosen is valid. It's already NG style, but that shouldn't hurt too much, right?
  • Test Normalize/Denormalize Test
    Fixed an error in EntityReferenceItem.
  • Test Entity UUIDs
    Revision ID's weren't properly tested in EntityUUIDTest::assertCRUD().
  • Test Entity Field API
    Looking at the code in EntityFieldTest::testEntityConstraintValidation() I would assume EntityWrapper::setValue($node) should throw an exception since EntityWrapper::definition['constraints']['EntityType'] defines entity_test but that's not the case.
    My validation knowledge is to limited to make quick progress, any hints?

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-95.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
178.92 KB
10.22 KB

Merged with latest 8.x and field-node-berdir (http://drupalcode.org/sandbox/fago/1497344.git/commit/96612319aeef306f82...)

  • by berdir: Fixed SearchCommentCountToggleTest, had invalid node params.
  • Fix for handling yet undefined properties in EntityFormControllerNG::buildEntity() - follow-up of #61
  • Test Node translation UI
    Failures because Node is now a EntityBCDecorator. While EntityNG is handled, EntityBCDecorator wasn't.
    I've added now some $entity = $entity->getOriginalEntity(); to unwrap the entity and use the EntityNG-Handling.
    Should we create dedicated test entity types? A non-NG and an EntityBCDecorator one?
    And it seems like there was some tangling with the save button naming - or did I miss a hidden status change that caused that tangling?
    I've made NodeTranslationUITest::getFormSubmitAction() status sensitive now.
  • Test Node translation UI
    Failures because Node is now a EntityBCDecorator. While EntityNG is handled, EntityBCDecorator wasn't.
    I've added now some $entity = $entity->getOriginalEntity(); to unwrap the entity and use the EntityNG-Handling.
    Should we create dedicated test entity types? A non-NG and an EntityBCDecorator one?
    And it seems like there was some tangling with the save button naming - or did I miss a hidden status change that caused that tangling?
    I've made NodeTranslationUITest::getFormSubmitAction() status sensitive now.

The attached interdiff doesn't reflect the changes from the merges! (Otherwise it would be huuuuge)

Status: Needs review » Needs work
Issue tags: -WSCCI, -D8MI, -sprint, -language-content, -Entity Field API

The last submitted patch, node-ng-1818556-97.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

#97: node-ng-1818556-97.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-97.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review

#97: node-ng-1818556-97.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +D8MI, +sprint, +language-content, +Entity Field API

The last submitted patch, node-ng-1818556-97.patch, failed testing.

das-peter’s picture

I've no clue why the heck it's failing.

[11:48:57] Command [/usr/bin/php ./core/scripts/run-tests.sh --concurrency 8 --php /usr/bin/php --url 'http://drupaltestbot659-mysql/checkout' --keep-results --all 2>&1] failed
  Duration 1 seconds
  Directory [/var/lib/drupaltestbot/sites/default/files/checkout]
  Status [255]
 Output: [PHP Fatal error:  Class 'Insert' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php on line 196

Fatal error: Class 'Insert' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php on line 196].
[11:48:57] Encountered error on [review], details:
array (
  '@reason' => 'failed during invocation of run-tests.sh',
)

That problem nor the class Insert has a meaning to me. The patch doesn't event contain the word Insert as class declaration :|

Question
Test Multilingual fields
Is this test-case suitable at all? It was basically a test to check if fields of non-NG entity types can be translated properly, right?
Shall we introduce a dedicated test entity to test this?

das-peter’s picture

Status: Needs work » Needs review
FileSize
189.73 KB
3.51 KB

Fixes by berdir, thanks!

  • Fixed missing use in NodeTranslationUITest
  • Hugly fix for SearchMultilingualEntityTest and revert f944ef

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-104.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
193.68 KB
4.79 KB

Last patch before I go to sleep:

  • Test Taxonomy term index
    Looks like calling EntityBCDecorator::__unset() didn't make the property act like it isn't set but as it is set to empty. Using the function should cause the property to be ignored on save and not removing the data.
    This isset / unset thing is really tricky :| But maybe I just missed something :D
  • Test RDFa markup
    rdf_field_attach_view_alter() used $item['taxonomy_term'] instead $item['entity']. Changed the usage there and in some other locations.
    Not sure yet if TaxonomyAutocompleteWidget::formElement() needs to be adjusted too.
  • Test Field: Field handler
    An empty field was / is passed to field_view_field() not sure if it shouldn't passed at all or if, for the sake of defensive coding, the function itself should check the field has values in the requested language.
    I went for the later now. Other opinions?
  • Test Node: User has revision Filter
    Explicitly setting revision_uid failed. Adjusted check in NodeStorageController::preSaveRevision().

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-106.patch, failed testing.

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -219,7 +219,9 @@ public function __isset($name) {
-    $value = array();
+    // Unset should act like the property isn't set and thus ignored. Following
+    // structure will provide that behaviour with EntityBCDecorator::__isset().
+    $value = array(LANGUAGE_NOT_SPECIFIED => NULL);

True, it should be set to NULL though. This is the value of the untranslated field only, so no reason to key by langcode?

+++ b/core/modules/field/field.module
@@ -944,7 +944,10 @@ function field_view_field(EntityInterface $entity, $field_name, $display_options
     $display_langcode = field_language($entity, $field_name, $langcode);
-    $items = $entity->{$field_name}[$display_langcode];
+    $items = array();
+    if (isset($entity->{$field_name}[$display_langcode])) {
+      $items = $entity->{$field_name}[$display_langcode];

This does not look right to me. Should field_language() make sure to return a langcode that works? Looks like there might be a problem with it and BC?

More trouble with EntityBCDecorator::__isset() an empty array has to be considered as set, even if an array with only NULL values has to be considered as not set.

That does not make sense to me, why should an array with only NULL values should be not set? Because of the default values we have in entity-ng here? True, with EntityNG we have those values to be set by default, but they are set to empty by default. Does that cause problems?

fago’s picture

Also #106 contains quite some unrelated hunks - please check the whole patch.

fago’s picture

Defined default values weren't assigned in field_populate_default_values() because EntityBCDecorator::__get() initializes the fields data array with NULL values. Thus the check in EntityBCDecorator::__isset() always returned TRUE.

hm, I think the better fix would be to fix field API to *always* set default values on hook_entity_create(). There is no reason not to and it's the only situation where we need to set defaults - so we do not need an isset() checks here any more?

fago’s picture

Thus, having isset($entity->field) being TRUE by default is not nice, but a good performance improvement. Still the field is correctly "empty", so there shouldn't be any harm in it.

The only possible problem I could see is during editing if widget show an extra empty item-form for the empty item, e.g. it shows two empty item forms instead of one. I guess we could fix that by removing empty items when starting editing (=when building the entity form the first time). This should be generally possible within the entity form controllers. Again not very nice, but imho a reasonable small work-a-round for better performance.

das-peter’s picture

Status: Needs work » Needs review
FileSize
185.76 KB
3.76 KB

Isset requirements / problems as far as I understand:

  1. We need the empty initialization in EntityBCDecorator::__get() to be able to return a reference and for performance reasons.
  2. Since the initialization sets a whole data structure calling EntityBCDecorator::__get() on a non yet set property will change the return of EntityBCDecorator::__isset() for that property - that has to be avoided.
  3. A call to EntityBCDecorator::__unset() doesn't change the return of EntityBCDecorator::__isset() of non-fields to FALSE - that should be changed.

My idea was to add a tracking, for empty initialized properties, that can be used in EntityBCDecorator::__isset() to return the appropriate value.
Tracking is done in a protected class variable which is used in the methods __get(),__set(),set(),__isset(),__unset() of EntityBCDecorator.
EntityBCDecorator::__unset(): I'm a bit confused there, I'm not sure if the behaviour between properties and fields really has to be different as I did implement this now.
However, local testing looks not to bad thus let's see what the test-bot says.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-112.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
  • Test Options field UI
    The test fails because the EntityBCDecorator doesn't recognize fields of this type.
    This happens because field_entity_field_info() doesn't return a field definition if there's no field item class defined. Thus DatabaseStorageController::getFieldDefinitions() never knows about fields of the list type.
    We need to push:
    http://drupal.org/node/1732730
das-peter’s picture

  • Test Translation functionality
    translation_test.module implements translation_test_node_insert() that saves the $node object.
    Of course drupal_write_record() isn't the thing to use anymore.
    However, what's the reason we need to save the node again in this hook? This feels somehow odd, especially singe all tests pass as well when I don't call $node->save().
das-peter’s picture

  • Test Entity Test translation UI and Comment translation UI
    "Missing" $entity->updateOriginalValues(); after making the changes in EntityTranslationControllerNG::removeTranslation().
  • Test REST: Update resource
    "Missing" $entity->updateOriginalValues(); after making the changes in EntityResource::patch().

What?!? I'm a bit confused by the need to call EntityNG::updateOriginalValues() and / or wasn't this already part of the storage controller somewhen?

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-116.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
185.95 KB

Re-roll

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-118.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
188.65 KB
3.18 KB
  • by berdir: Test EntityReferenceAutoCreateTest and EntityReferenceAdminTest.
    Although that ids function was ugly, it had nothing to do with that.
    Fields for entities without bundles (although the check there is wrong, will fail ugly if an entity has bundles and one bundle == entity_type) are in 'definitions', fields for entities with bundles are in 'optional'.
    entity_reference_entity_field_info() always added the settings to definitions which worked fine for entity_test but not node.
    Then definitions contained a partial info and at the bottom, where the optional fields are added with += it didn't overwrite the existing key.
    I've changed the entity reference hook to an alter hook and checked where I need to add it, which is ugly, but the whole function should go anyway, we need a solution to add additional field data/settings right in field_entity_field_info()
  • by berdir: Test EntityReferenceSelectionSortTest.
    Fixed creation of test content.
  • Test Entity Field API.
    The entity wrapper didn't update EntityWrapper::$entityType when a non new entity was set in EntityWrapper::setValue().
    Thus setting a node still validated as 'entity_test' entity.

What's left:

  1. Node create: depends on #1872690: Exception: Serialization of 'Closure' is not allowed in serialize
  2. OptionsFieldUITest: depends on #1839064: Implement the new entity field API for the list field type
Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-120.patch, failed testing.

das-peter’s picture

Assigned: das-peter » Unassigned

Now this definitely needs reviews :)

Open points / questions:

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI, -D8MI, -sprint, -language-content, -Entity Field API

#120: node-ng-1818556-120.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +D8MI, +sprint, +language-content, +Entity Field API

The last submitted patch, node-ng-1818556-120.patch, failed testing.

das-peter’s picture

Oh, even less fails than expected! :)
I expected Node create to fail too.
And the test Multilingual fields (NodeFieldMultilingualTestCase) is maybe even obsolete.
So, even if not green yet this looks promising.

plach’s picture

I'll have a look to the last test tonight, if no one beats me to it.

Awesome work, guys!

YesCT’s picture

+++ b/core/modules/node/node.api.phpundefined
@@ -256,7 +256,7 @@ function hook_node_grants($account, $op) {
- * @param Drupal\node\Node $node
+ * @param \Drupal\Core\Entity\EntityInterface $node

@@ -266,7 +266,7 @@ function hook_node_grants($account, $op) {
-function hook_node_access_records(Drupal\node\Node $node) {
+function hook_node_access_records(\Drupal\Core\Entity\EntityInterface $node) {

@@ -317,7 +317,7 @@ function hook_node_access_records(Drupal\node\Node $node) {
- * @param Drupal\node\Node $node
+ * @param \Drupal\Core\Entity\EntityInterface $node

@@ -329,7 +329,7 @@ function hook_node_access_records(Drupal\node\Node $node) {
-function hook_node_access_records_alter(&$grants, Drupal\node\Node $node) {
+function hook_node_access_records_alter(&$grants, Drupal\Core\Entity\EntityInterface $node) {

@@ -459,14 +459,14 @@ function hook_node_operations() {
- * @param Drupal\node\Node $node
+ * @param \Drupal\Core\Entity\EntityInterface $node
...
-function hook_node_predelete(Drupal\node\Node $node) {
+function hook_node_predelete(\Drupal\Core\Entity\EntityInterface $node) {

why do these, and a bunch of other function definitions not just say (EntityInterface $...) ?

Are they missing a use statement?

YesCT’s picture

Some of these comments are temporary, so I dont know how picky we want to be with them...

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -46,13 +46,31 @@ class EntityBCDecorator implements IteratorAggregate, EntityInterface {
+   * @param array
+   *   An array of field definitions.
...
+  function __construct(EntityNG $decorated, array &$definitions) {

should be:
@param array &$definitions

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -96,21 +122,36 @@ public function &__get($name) {
+      // Allow accessing field values in entity default language other than

in an entity

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -96,21 +122,36 @@ public function &__get($name) {
+      // LANGUAGE_DEFAULT by mapping the values to LANGUAGE_DEFAULT. This is
+      // necessary as EntityNG does key values in default language always with
+      // LANGUAGE_DEFAULT while field API expects them to be keyed by langcode.

@@ -119,24 +160,42 @@ public function &__get($name) {
+        // This is necessary as EntityNG does key values in default language always
+        // with LANGUAGE_DEFAULT while field API expects them to be keyed by
+        // langcode.

// necessary as EntityNG always keys default language values with

+++ b/core/lib/Drupal/Core/TypedData/Type/Language.phpundefined
@@ -8,8 +8,10 @@
+

extra newline. remove.

+++ b/core/modules/book/book.moduleundefined
@@ -242,7 +242,7 @@ function _book_outline_remove_access(Node $node) {
- * @param Drupal\node\Node $node
+ * @param \Drupal\Core\Entity\EntityInterface $node
...
 function _book_node_is_removable($node) {

change function declaration to also have type EntityInterface

+++ b/core/modules/entity_reference/entity_reference.moduleundefined
@@ -35,24 +35,27 @@ function entity_reference_field_info() {
+ * Implements hook_entity_field_info_alter().
...
+function entity_reference_entity_field_info_alter(&$property_info, $entity_type) {

no type hints?

YesCT’s picture

re #128 @berdir says hooks always use fully qualified, so they can be cut and paste.
See "API documentation (in .api.php files) should use full class names. Note that if a class is used more than once in multiple hook signatures, it must still be "use"ed, and then only the short names of the class should be used in the function." in namespaces standards http://drupal.org/node/1353118

YesCT’s picture

+++ b/core/modules/node/node.moduleundefined
@@ -412,7 +411,7 @@ function node_type_get_label($name) {
+ * @param \Drupal\Core\Entity\EntityInterface $node

@@ -912,7 +911,7 @@ function node_hook($type, $hook) {
+ * @param \Drupal\Core\Entity\EntityInterface $node

these seem to be missing an update to their respective function declarations

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/TrackerAttributesTest.phpundefined
@@ -71,10 +71,10 @@ function testAttributesInTracker() {
+   * @param EntityInterface $node

use fully qualified \Drupal...

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1104,7 +1101,7 @@ function taxonomy_field_formatter_view(EntityInterface $entity, $field, $instanc
   // Terms whose tid is 'autocreate' do not exist
-  // yet and $item['taxonomy_term'] is not set. Theme such terms as
+  // yet and $item['entity'] is not set. Theme such terms as
   // just their name.

re-wrap to 80 chars (clearly this is optional)

das-peter’s picture

Status: Needs work » Needs review
FileSize
189.78 KB
6.85 KB

@YesCT: Thank you very much for the feedback. Patch updated.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-132.patch, failed testing.

andypost’s picture

plach’s picture

Assigned: Unassigned » plach
Status: Needs work » Needs review
FileSize
194.3 KB
4.82 KB

This should fix the last failures (!)

cfe65da Fix tracker tests.
da7a76c Fix multilingual fields tests.

As @das-peter pointed out the Multilingual fields test is obsoleted by the Entity Field API, which always uses the LANGUAGE_DEFAULT language code for the original values. Instead of further messing with the BC decorator, I just converted the test to NG, which shows how much simpler dealing with field language has become :)

plach’s picture

Assigned: plach » Unassigned
Berdir’s picture

Status: Needs review » Needs work

Awesome, this is green.

Will try to run my Convert test_entity to entity_test issue with all the BC decorator changes as that one has tons of field API tests that should help verify that the changes are correct and hopefully fix some of my tests too.

Review below but this looks quite good already IMHO. I think we need a plan on how we want to get rid of the temporary stuff, see comment below and then this should get in ASAP.

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -119,24 +160,42 @@ public function &__get($name) {
+    // The property revision was used to create a new revision - do so as well.
+    // @TODO Did this apply only to node or for other entity types too?
+    if ($name == 'revision') {
+      $this->setNewRevision($value);

I think we should find a way to get rid of this, possibly override the form controller to map $form_state['values']['revision'] directly to setNewRevision(). This already happens a bit later (during submit/save).

The Custom block entity should already handle this, as it is NG and supports revisions.

+++ b/core/lib/Drupal/Core/Entity/Field/FieldItemBase.phpundefined
@@ -37,6 +37,15 @@
 
   /**
+   * Holds any non-property values that get set on the object.
+   *
+   * @todo: Remove or refactor once EntityNG conversion is complete.
+   *
+   * @var array
+   */
+  protected $extraValues = array();

Do you know which values are added additionally?

An alternative of this might be to simply define them explicitly, that worked fine with the additional_key in field_test. However, it also affects some tests that compare whole arrays.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReferenceItem.phpundefined
@@ -86,10 +86,35 @@ public function setValue($values) {
 
   /**
+   * Overrides \Drupal\Core\Entity\Field\FieldItemBase::__set().
+   */
+  public function __set($name, $value) {
+    $name = ($name == 'value') ? 'target_id' : $name;
+    parent::__set($name, $value);
+  }
+
+  /**
+   * Overrides \Drupal\Core\Entity\Field\FieldItemBase::__get().
+   */
+  public function __get($name) {
+    $name = ($name == 'value') ? 'target_id' : $name;
+    return parent::__get($name);

Hm, the same thing should allow me to fix DateTimeItem.

I still think there should be standardized way to return the "storage value".

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityWrapper.phpundefined
@@ -65,6 +72,9 @@ public function __construct(array $definition, $name = NULL, ContextAwareInterfa
   public function getValue() {
+    if (isset($this->newEntity)) {
+      return $this->newEntity;

Interesting, that is exactly the fix that I suggest in the file field type issue to support things like prepare_view() load improvements. So we should look into making this the default behavior imho, but tha can happen in a follow-up issue.

+++ b/core/modules/field/lib/Drupal/field/Tests/Views/ApiDataTest.phpundefined
@@ -66,8 +64,8 @@ function setUp() {
         // @TODO Write a helper method to create such values.
-        'field_name_0' => array($langcode => array((array('value' => $this->randomName())))),
-        'field_name_2' => array($langcode => array((array('value' => $this->randomName())))),
+        'field_name_0' => array((array('value' => $this->randomName()))),
+        'field_name_2' => array((array('value' => $this->randomName()))),

Wondering if we still need that @todo, there's not that much left that a helper function could do.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.phpundefined
@@ -88,7 +78,7 @@ class Node extends Entity implements ContentEntityInterface {
    *
    * @var string
    */
-  public $langcode = LANGUAGE_NOT_SPECIFIED;

The @var definitions should probably be updated, see Comment.php.

+++ b/core/modules/node/node.api.phpundefined
@@ -1332,7 +1332,7 @@ function hook_validate(Drupal\node\Node $node, $form, &$form_state) {
  */
-function hook_view(\Drupal\node\Plugin\Core\Entity\Node $node, \Drupal\entity\Plugin\Core\Entity\EntityDisplay $display, $view_mode) {
+function hook_view(\Drupal\Core\Entity\EntityInterface $node, \Drupal\entity\Plugin\Core\Entity\EntityDisplay $display, $view_mode) {
   if ($view_mode == 'full' && node_is_page($node)) {

We should probably write up an issue summary and document the approach here, overview of the changes and what is temporary and what not (And a plan to remove the temporary things, not yet sure what's the best approach there).

Otherwise, all these type hint changes might be confusing for reviewers.

+++ b/core/modules/node/node.moduleundefined
@@ -1409,7 +1414,7 @@ function node_search_execute($keys = NULL, $conditions = NULL) {
-      'date' => $node->get('changed', $item->langcode),
+      'date' => $node->changed,

I am not sure why this was langcode specific as changed is not yet translatable.

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.phpundefined
@@ -57,33 +57,35 @@ function setUp() {
       array(
         'type' => 'page',
-        'body' => array(
-          'en' => array(array('value' => $this->randomName(32), 'format' => $default_format)),
-          'hu' => array(array('value' => $this->randomName(32), 'format' => $default_format)),
-        ),
+        'body' => array(array('value' => $this->randomName(32), 'format' => $default_format)),
         'langcode' => 'en',
...
+    // Add a single translation to the second node.
+    $translation = $this->searchable_nodes[1]->getTranslation('hu');
+    $translation->body->value = $this->randomName(32);
+    $this->searchable_nodes[1]->save();

This might be the only case where we actually created nodes with multiple initial languages.

The change isn't very nice but I think such cases are very rare and don't need to be supported directly.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.phpundefined
@@ -583,4 +583,28 @@ protected function assertComputedProperties($entity_type) {
+  public function testBCDecorator() {
+    // Test using comment subject via the BC decorator.
+    module_enable(array('node', 'comment'));
+    $node = $this->drupalCreateNode();

Can we use entity_create() for the node as well?

This will conflict with my DUBT patch for the entity tests and I would like to keep that conflict as small as possible. Maybe that should be in a different test class then we can remove the whole thing and it won't conflict. Isn't there already a test class for the BC decorator?

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1068,10 +1068,7 @@ function taxonomy_field_validate(EntityInterface $entity = NULL, $field, $instan
 function taxonomy_field_is_empty($item, $field) {
-  if (!is_array($item) || (empty($item['tid']) && (string) $item['tid'] !== '0')) {
-    return TRUE;
-  }
-  return FALSE;
+  return empty($item['tid']) && empty($item['entity']);

Are we sure this change doesn't break not yet converted entity types? What happens if you add a taxonomy field to users and try autocreate?

Same with the other taxonomy_term/entity changes.

amateescu’s picture

What happens if you add a taxonomy field to users and try autocreate?

Same with the other taxonomy_term/entity changes.

We shouldn't care about that, #1847596: Remove Taxonomy term reference field in favor of Entity reference is taking care of it.

Berdir’s picture

@amateescu: That's not what I meant.

It' only temporarly broken anyway, it would work again once all entity types are converted, whether it is replaced by ER or not.

But until that happened, or it is replaced with ER, it would be broken.

longwave’s picture

+ protected $intializedEmpty = array();
Should be $initializedEmpty?

fago’s picture

Amazing to see this green!

Do you know which values are added additionally?

An alternative of this might be to simply define them explicitly, that worked fine with the additional_key in field_test. However, it also affects some tests that compare whole arrays.

Well, I think it would not hurt to support writing arbitrary stuff into $entity->field[0]->custom ? It could be still ignored by all the typed-data API and e.g. only returned by $this->getValue() and support by $this->setValue() as the plain values of a field item while ->getPropertyValues() would be "clean" ?

For that, we could re-factor the field-item class with #1869562: Avoid instantiating EntityNG field value objects by default and have it all in the protected $values.

Interesting, that is exactly the fix that I suggest in the file field type issue to support things like prepare_view() load improvements. So we should look into making this the default behavior imho, but tha can happen in a follow-up issue.

Yes, let's take care of that in #1868004: Improve the TypedData API usage of EntityNG.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -96,7 +96,7 @@ public function __construct($entityType) {
-    $bundle = $this->bundleKey ? $values[$this->bundleKey] : FALSE;
+    $bundle = $this->bundleKey && isset($values[$this->bundleKey]) ? $values[$this->bundleKey] : FALSE;

Is that change necessary? If the entity has a bundle it's required to be passed into entity_create() - so throwing a notice is the least it should do then... an exception would be even better.

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -46,13 +46,31 @@ class EntityBCDecorator implements IteratorAggregate, EntityInterface {
+   * Track empty initialized properties to return an appropriate value in
+   * EntityBCDecorator::__isset().
+   *
+   * @var array
+   */
+  protected $intializedEmpty = array();

Can we add a comment on why we need that? (Actually I still miss the point here ;) Why does setting the field to NULL in unset not cut it?

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -144,15 +203,23 @@ public function __set($name, $value) {
+    // When accessing values for entity properties that have been converted to
+    // an entity field, provide direct access to the plain value. This makes it
+    // possible to use the BC-decorator with properties; e.g., $node->title.
+    if (isset($this->definitions[$name]) && empty($this->definitions[$name]['configurable'])) {
+      // Ensure it's really recognized as unset in EntityBCDecorator::__isset().

I don't see how the comments relates to the code below?

+++ b/core/modules/taxonomy/taxonomy.module
@@ -1068,10 +1068,7 @@ function taxonomy_field_validate(EntityInterface $entity = NULL, $field, $instan
-  if (!is_array($item) || (empty($item['tid']) && (string) $item['tid'] !== '0')) {
-    return TRUE;
-  }
-  return FALSE;
+  return empty($item['tid']) && empty($item['entity']);

This assumes $item is an array what the old code did not?

+++ b/core/modules/taxonomy/taxonomy.module
@@ -1138,7 +1134,7 @@ function taxonomy_field_formatter_view(EntityInterface $entity, $field, $instanc
-          'value' => $item['tid'] != 'autocreate' ? $item['taxonomy_term']->label() : $item['name'],
+          'value' => $item['tid'] != 'autocreate' ? $item['entity']->label() : $item['name'],

I'm looking forward to cleaning this up by using $item->entity always - even in case of auto-creation ;-)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
4.96 KB
197.3 KB

Fixed:
Removing todo mentioned in #137

The @var definitions should probably be updated, see Comment.php.

updated.

This assumes $item is an array what the old code did not?

in #141 updated.

plach’s picture

The attached patch should address the pending issues form the reviews above. I only omitted the $initializedEmpty stuff as @das-peter can provide better feedback on that. Since the patch reverts some changes that were likely introduced to fix some tests I expect some failures, which I'll fix asap.

c80fe6d Issue #1818556: Removed $this->drupalCreateNode() from entity field test.
7da0a24 Issue #1818556: Removed revision property special-casing.
0858244 Issue #1818556: Fixed typo in the BC decorator.
c326ca4 Issue #1818556: Reverted behavior change when creating a bundless entity.
plach’s picture

Some notes:

Review below but this looks quite good already IMHO. I think we need a plan on how we want to get rid of the temporary stuff, see comment below and then this should get in ASAP.

I think a possible approach to complete the conversion might be to start converting every module in its own issue. This would allow us to incrementally provide all the node field definitions and thus get rid of the BC decorator instances in the related places. Also #1498674: Refactor node properties to multilingual might help with that.

I still think there should be standardized way to return the "storage value".

Yes, I totally agree. When working on the early patches here I often found myself wishing for an automated way to map from ->value to ->target_id and viceversa. Probably a 'default property' key in the field definition would be enough for this.

I am not sure why this was langcode specific as changed is not yet translatable.

This was probably paving the way for multilingual support for search code. Anyway this line will just become $node->changed->value once we convert that part and we are done with #1810370: Entity Translation API improvements.

The change isn't very nice but I think such cases are very rare and don't need to be supported directly.

Not sure about this: field_entity_create() is already supporting this scenario, thus I think entity_create() should too. In another issue obviously.

Are we sure this change doesn't break not yet converted entity types? What happens if you add a taxonomy field to users and try autocreate

Manually tested this use case and everything worked well.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Updated the issue summary as suggested in #137.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-143.patch.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
198.58 KB
760 bytes

Fixed the exception.

das-peter’s picture

Just adjusted comment in EntityBCDecorator::__unset() to actually make sense in that context.

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -96,21 +122,36 @@ public function &__get($name) {
+      $value = $this->decorated->values[$name][LANGUAGE_DEFAULT];
+      if (is_array($value)) {
+        $value = !empty($value[0]) ? reset($value[0]) : NULL;

hm, this seems to fail to return a reference on the proper value. That means a $node->property['foo'] = '3'; would fail?

das-peter’s picture

The issue with EntityBCDecorator::__isset():
In D7 an entity was often an instance of stdClass - thus you were able to simply add properties as you liked e.g. $node->foo = 'bar'.
The entityNG approach in D8 now needs definition for all properties, thus the EntityBCDecorator had to workaround this to be really backward compatible.
So if you access EntityBCDecorator->foo which is handled by EntityBCDecorator::__get() it tries to find the appropriate definition and if there's none, it initializes an internal data-structure for foo to be able to work with references.
The initialization looks like this $this->decorated->values['foo'][LANGUAGE_DEFAULT][0]['value'] = NULL;.
Thus EntityBCDecorator::__get() returns array('value' => NULL) for a basically non-existing property. (Don't ask me why, we had some issues there and that was a solution ;) )

Now EntityBCDecorator::__isset() does nothing else as calling EntityBCDecorator::__get() and then run an isset() on the returned value. But wait(!), this will initialize non existing properties and the return value will be array('value' => NULL)! Which means the isset() check in EntityBCDecorator::__isset() will always evaluate to TRUE for non-existing properties!

One could argue why not simply check properties with isset($value[0]['value']) then?
Well, maybe EntityBCDecorator->foo will contain array('snafu' => 'bar'), so isset($value[0]['value']) would then return FALSE even if a proper value is set.

Easiest way, I could think of, to work around this was to track empty initialized or explicitly unset properties and check the tracking in EntityBCDecorator::__isset().

das-peter’s picture

Forget the comment above (will correct it later).
I found some other related stuff and will re-roll the whole thing again.
To do so here's a patch with some stuff reverted, and some changes that hopefully keep the patch green.
No interdiff yet, I just need the results of the test-bot to see where I can reproduce errors - my machine is to busy to run all the tests ;)

das-peter’s picture

Nice, the proof of concept patch is green :)
Slightly adjusted patch attached.
I'm not sure how everything is related, looks like a lucky punch ;P, but I fixed the missing reference for the cases in which a defined property was initialized empty.
Further I ensured it worked not just with properties that have 'value' as key for the data.
Open question is if properties with more than just one data key will work as well.
As far as I know there's a discussion ongoing about define the "default key" to map 'value' always to that.

Status: Needs review » Needs work
Issue tags: -WSCCI, -D8MI, -sprint, -language-content, -Entity Field API

The last submitted patch, node-ng-1818556-152.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review

#152: node-ng-1818556-152.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-152.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review

#152: node-ng-1818556-152.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +D8MI, +sprint, +language-content, +Entity Field API

The last submitted patch, node-ng-1818556-152.patch, failed testing.

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.phpundefined
@@ -583,4 +583,32 @@ protected function assertComputedProperties($entity_type) {
+  public function testBCDecorator() {
+    // Test using comment subject via the BC decorator.
+    module_enable(array('node', 'comment'));
+    $node = entity_create('node', array(

This needs to use $this->moduleEnable() now.

das-peter’s picture

Status: Needs work » Needs review
FileSize
197.7 KB

@Berdir: Thanks for the hint!
Patch re-rolled against latest 8.x and made the changes mentioned by berdir in #158.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-159.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
198.43 KB
1.37 KB

*blargh* more adjustments needed to get test (EntityFieldTest::testBCDecorator() running. Now the setup for the test should work.
Test failed locally too, now it passes.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-161.patch, failed testing.

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.phpundefined
@@ -22,7 +22,7 @@ class EntityFieldTest extends EntityUnitBaseTest  {
    */
-  public static $modules = array('filter', 'text', 'node');
+  public static $modules = array('filter', 'text', 'node', 'comment');
 
   public static function getInfo() {
     return array(
@@ -36,6 +36,7 @@ public function setUp() {

@@ -36,6 +36,7 @@ public function setUp() {
     parent::setUp();
     $this->installSchema('user', array('users_roles', 'users_data'));
     $this->installSchema('node', array('node', 'node_revision', 'node_type', 'node_access'));
+    $this->installSchema('comment', array('comment', 'node_comment_statistics'));
     $this->installSchema('entity_test', array(
       'entity_test_mul',
       'entity_test_mul_property_data',
@@ -620,6 +621,7 @@ protected function assertComputedProperties($entity_type) {

@@ -620,6 +621,7 @@ protected function assertComputedProperties($entity_type) {
   public function testBCDecorator() {
     // Test using comment subject via the BC decorator.
     $this->enableModules(array('node', 'comment'));
+    $this->createUser();
     $node = entity_create('node', array(

I really think we should move the BC test into a separate class. This adds the schema installation to all other tests of that huge test class too, making it unecessarly slower.

YesCT’s picture

@Berdir That sounds like a follow-up to me. Are you ok with that? I can open it.

Berdir’s picture

As discussed, the test failed and needs to be fixed anyway, moving to a different file is not that hard.

Also, I'm not suggesting to move it because of those additional dependencies. Those are just another sign why that test doesn't belong in that test class. It's a test for a different API, an API that is only temporary. Being able to just remove a file is easier than looking for test methods in an already large test class.

das-peter’s picture

Status: Needs work » Needs review
FileSize
199.57 KB
5.01 KB

I'm in favor of a separate class too. Thus here's an updated patch.
I've added some more assertions for the EntityBCDecoratorTest as well.
Unfortunately I don't expect this to become green, even if it does on my local system.
The same happened with the patch in #161.
(Because of Windows?!? Normally it's the other way around ;) )

Would be great if someone could just run the Entity Field API and the new Entity Backward Compatibility Decorator tests to see if one fails.

Berdir’s picture

Thanks!

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityBCDecoratorTest.phpundefined
@@ -0,0 +1,87 @@
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Entity\Field\FieldInterface;
+use Drupal\Core\Entity\Field\FieldItemInterface;
+use Drupal\Core\TypedData\TypedDataInterface;
...
+    // Install required default configuration for filter module.
+    $this->installConfig(array('system', 'filter'));
...
+    // Test using comment subject via the BC decorator.
+    $this->enableModules(array('node', 'comment'));

use statements are unused, config isn't needed it seems and enabling the modules once is enough :)

das-peter’s picture

Cleanup according to #167.

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-168.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
815 bytes
199.24 KB

I really don't get it why the test doesn't fail locally. I've added the schema role_permission to the setup method now. Hope this helps.

Berdir’s picture

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -56,7 +56,7 @@ protected function attachLoad(&$records, $load_revision = FALSE) {
-      $node = node_load($values['nid']);
+      $node = node_load(is_object($values['nid']) ? $values['nid']->value : $values['nid']);

I believe this is one of the early changes that I made before the BC decorator really worked, is this still necessary?

+++ b/core/modules/node/node.moduleundefined
@@ -2668,7 +2673,7 @@ function node_access($op, $node, $account = NULL, $langcode = NULL) {
-    elseif (is_object($node) && $op == 'view' && $node->get('status', $langcode)) {
+    elseif (is_object($node) && $op == 'view' && !empty($node->get('status', $langcode)->value)) {

Interesting that this works here but $node->get('changed, $langcode) in search didn't. Maybe this is never called with a non-default langcode? Because according to the change I had to make there, this explodes if called for a properties that is not actually translatable (not sure if it should or just fallback to the language neutral one)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityBCDecoratorTest.phpundefined
@@ -0,0 +1,78 @@
+ * Definition of Drupal\system\Tests\Entity\EntityBCDecoratorTest.

Forgot to mention that before. Should be "Contains \Drupal\..."

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -614,6 +614,8 @@ function translation_entity_form_alter(array &$form, array &$form_state) {
+      // @todo remove once EntityBCDecorator is gone.
+      $entity = $entity->getOriginalEntity();

+++ b/core/modules/translation_entity/translation_entity.pages.incundefined
@@ -238,6 +238,8 @@ function translation_entity_edit_page(EntityInterface $entity, Language $languag
+  // @todo remove once EntityBCDecorator is gone.
+  $entity = $entity->getOriginalEntity();

That @todo is kinda pointless, as this is true for all getOriginalEntity() and getBCEntity() calls.

But this looks really good to me. Maybe @fago can have another look through the BC Decorator stuff because I don't feel comfortable RTBC'ing that stuff.

plach’s picture

Interesting that this works here but $node->get('changed, $langcode) in search didn't.

That code should be $node->get('changed, $langcode)->value to work the same way I guess.

fago’s picture

I had a look at the patch and in particular the changes at the BC-decorator. BC-decorator changes look great to me, good job!

+++ b/core/lib/Drupal/Core/Entity/EntityFormControllerNG.php
@@ -81,6 +81,11 @@ public function buildEntity(array $form, array &$form_state) {
+      elseif ($entity instanceof EntityBCDecorator) {
+        // Handle yet undefined properties.
+        // @todo Remove once EntityBCDecorator is removed.

This part does not make much sense to me. It's in the NG-form controller, so why should it get a BC decorator? It should be able to work with the NG-entity.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReferenceItem.php
@@ -86,10 +86,35 @@ public function setValue($values) {
+  public function __set($name, $value) {
+    $name = ($name == 'value') ? 'target_id' : $name;
+    parent::__set($name, $value);

Can we comment on why and when this is necessary? I'd assume it's because of the storage controller assuming it can go with ->value.

That actually confuses me a bit, as EntityReferenceItem already defines it as 'target_id', but the NG-storage works with "value". Has its storage been broken without breaking tests?

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityWrapper.php
@@ -65,6 +72,9 @@ public function __construct(array $definition, $name = NULL, ContextAwareInterfa
   public function getValue() {
+    if (isset($this->newEntity)) {
+      return $this->newEntity;

This creates potentially stale data we need to take care of. But that we can off-load to #1868004: Improve the TypedData API usage of EntityNG which is about to re-vamp this logic.

das-peter’s picture

I believe this is one of the early changes that I made before the BC decorator really worked, is this still necessary?

Actually I ran all comment tests with a conditional (is_object($values['nid'])) breakpoint set and it never was hit.
So I assume we could remove that - however I wasn't bold enough yet to do so.

Interesting that this works here but $node->get('changed, $langcode) in search didn't. Maybe this is never called with a non-default langcode? Because according to the change I had to make there, this explodes if called for a properties that is not actually translatable (not sure if it should or just fallback to the language neutral one)

I've checked this again. Was really confusing thus I changed the code now, I hope it's more obvious what's going on there now - even if it looks horrible... ;)
What I'm a bit concerned about is the fact that we still have to deal with node objects based on stdClass there.

Forgot to mention that before. Should be "Contains \Drupal\..."

Changed.

That @todo is kinda pointless, as this is true for all getOriginalEntity() and getBCEntity() calls.

@todos removed.

This part does not make much sense to me. It's in the NG-form controller, so why should it get a BC decorator? It should be able to work with the NG-entity.

Well NodeFormController::buildEntity() passes over to EntityFormControllerNG::buildEntity() which uses $this->getEntity() and this is handled by NodeFormController::getEntity() which returns an instance of EntityBCDecorator.
As EntityBCDecorator and EntityNG are closely related I thought it would be better to keep the code in EntityFormControllerNG instead copying it into NodeFormController.
However, we could build something like an EntityBCDecoratorFormController.

Can we comment on why and when this is necessary? I'd assume it's because of the storage controller assuming it can go with ->value.
That actually confuses me a bit, as EntityReferenceItem already defines it as 'target_id', but the NG-storage works with "value". Has its storage been broken without breaking tests?

I've just ran the EntityReference tests with a breakpoint there, no hits. So I removed this mapping and still no EntityReference test-fails, let's see if another test fails.

This creates potentially stale data we need to take care of. But that we can off-load to #1868004: Improve the TypedData API usage of EntityNG

Anything to do here?

Status: Needs review » Needs work

The last submitted patch, node-ng-1818556-174.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
200.94 KB
1.08 KB

I've found the reason why there was:

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityReferenceItem.php
@@ -86,10 +86,35 @@ public function setValue($values) {
+  public function __set($name, $value) {
+    $name = ($name == 'value') ? 'target_id' : $name;
+    parent::__set($name, $value);

DatabaseStorageControllerNG::attachPropertyData() always used value to set the property data.
I've changed this now to base on the field property definition, however the current approach is just sophisticated guessing and nothing more. We really should fix that.
@fago As far as I remember there's already an issue (adjust property definition) for that, right?

fago’s picture

> @fago As far as I remember there's already an issue (adjust property definition) for that, right?
I don't think we have already an issue for that. Feel free to open one!

However, we could build something like an EntityBCDecoratorFormController.

Yes, it does not make much sense to me to specifically pass an $bc_entity into the NG controller - the NG controller is supposed to work with NG-entities... So if we want/need to work with a bc entity here, couldn't we just stay with the non-NG EntityFormController?

das-peter’s picture

> @fago As far as I remember there's already an issue (adjust property definition) for that, right?
I don't think we have already an issue for that. Feel free to open one!

I found the issue where I stumbled over this problem earlier: #1847588-30: Implement the new entity field API for the Entity-Reference field type
So basically we need a new issue for "Field API: Support a proper naming pattern in the schema columns", right?

Yes, it does not make much sense to me to specifically pass an $bc_entity into the NG controller - the NG controller is supposed to work with NG-entities... So if we want/need to work with a bc entity here, couldn't we just stay with the non-NG EntityFormController?

I don't think we can / should rely on the EntityFormController because there was EntityFormControllerNG::getEntity() just to use the BC decorator. I've now created a dedicated form controller. I think that's not the worst solution for now.

Berdir’s picture

#178: node-ng-1818556-178.patch queued for re-testing.

fago’s picture

So basically we need a new issue for "Field API: Support a proper naming pattern in the schema columns", right?

I think it's an issue of improving the default NG storagecontroller only, not field API. So, maybe something like "Implement a sane storage default mapping", e.g. map column names like user__target_id to $entity->user->target_id and vice versa.

I don't think we can / should rely on the EntityFormController because there was EntityFormControllerNG::getEntity() just to use the BC decorator

Yes, but that's the whole point. Why should the NG controller use a BC decorator if it's supposed to work with NG API? Cannoted we stay with the regular form controller and use it with BC - I mean that's the whole idea of a BC-entity: be able to use it with the old code.

das-peter’s picture

Issues for the "DatabaseStorageControllerNG stuff" created: #1936382: Implement a sane storage default mapping

EntityFormController stuff:
One problem is that EntityFormController::buildEntity() uses entity_form_submit_build_entity() which makes use of $entit->set($property_name, $value) and that tiggers an InvalidArgumentException if an unknown field is set.
We could fix this using something like following code in EntityBCDecorator::set():

    if (!isset($this->definitions[$property_name])) {
      return $this->__set($property_name, $value);
    }

Locally all node tests pass with this adjustment.
Is this approach ok? If so, shall I integrate something similar for EntityBCDecorator::get()?
As soon as I know how to proceed I'll create another patch.

das-peter’s picture

I've decided to give the in #181 mentioned approach a try. Here comes the patch.

fago’s picture

Good catch! So we make set() of the BC controller acting as previously, i.e. accepting everything. So that was the problem of the BC-entity!

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -219,6 +219,9 @@ public function access($operation = 'view', \Drupal\user\Plugin\Core\Entity\User
+    if (!isset($this->definitions[$property_name])) {
+      return $this->__get($property_name);

Maybe let's add a comment on what happens here, so that it's clear we make set() work with any $property_name given.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -947,6 +947,9 @@ protected function curlInitialize() {
+
+      $curl_options[CURLOPT_COOKIE] = 'XDEBUG_SESSION=DRUPAL_TESTING;';

Interesting :)

das-peter’s picture

Comments added, hope that single line is sufficient ;)

Ouch, forgot to remove the debugging artifact while creating the patches. Luckily this never made it into the pushed code.

YesCT’s picture

@das-peter I didn't see the debugging stuff in the interdiff. Can you point it out to me? I'm curious to see.

YesCT’s picture

Ah, @fago pointed it out in #183 It's not in the interdiff because this issue is using a sandbox so the interdiffs are not strictly between the patches posted here. (Also, the xdebug irc bot factoid has some info I'll look at in more detail.)

fago’s picture

I had another look at the whole patch and I think it's ready to move on (given it comes back green). Thoughts?

plach’s picture

Sounds good :)

das-peter’s picture

Same here, I think that looks quite good.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Patch is green. Team Europe is offline. Marking RTBC, and will leave it for a bit, but try and commit it tonight. EXCITING!!

webchick’s picture

Issue tags: +SprintWeekend2013
webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +needs profiling

Darn. As much as I want to commit this, I don't think I can do so until we have some data on how this affects performance. Tagging accordingly.

While we're waiting, some feedback:

+++ b/core/modules/book/book.admin.incundefined
@@ -110,7 +110,7 @@ function book_admin_settings_submit($form, &$form_state) {
-function book_admin_edit($form, $form_state, Node $node) {
+function book_admin_edit($form, $form_state, EntityInterface $node) {

That's really unfortunate. :( I understand that it gets us more flexibility, but at the cost of DX. :(

On IRC, msonnabaum suggested introducing a NodeInterface here instead, so we keep the flexibility, but also explain to people what it is they're dealing with. I don't think it's necessarily worth holding this issue up on that, but OTOH it would make find/replace much easier if we did it before we lose that granular information.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.phpundefined
@@ -43,64 +43,54 @@
-   * @var string
+   * @var \Drupal\Core\Entity\Field\FieldInterface
...
-   * @var integer
+   * @var \Drupal\Core\Entity\Field\FieldInterface

This, however, looks really bizarre to me. We're losing a LOT of data about expected input this way.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeEntityFieldQueryAlterTest.phpundefined
@@ -50,7 +50,7 @@ function setUp() {
-      $settings['body'][LANGUAGE_NOT_SPECIFIED][0] = $body;
+      $settings['body'][0] = $body;

Hunks like this, on the other hand, are very nice. :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityBCDecoratorTest.phpundefined
@@ -0,0 +1,78 @@
+/**
+ * @file
+ * Definition of \Drupal\system\Tests\Entity\EntityBCDecoratorTest.

Interesting. It's nice to have this, although hopefully we can delete it soon. :D

Berdir’s picture

Will try to do some profiling today.

Some answers to your feedback:

- I know why @msonnabaum suggested a NodeInterface but that doesn't help here. The reason we are doing it *here* is because of the EntityBCDecorator that we are working with that doesn't extend from Node and can't implement NodeInterface. This could be reverted after we replaced the BC Decorator. However, we might still need this for translations, see the discussion in #1391694-53: Use type-hinting for entity-parameters and below.

- The reason for the property change is that $node->status is now a class and has to be accessed as $node->status->value (after the BC decorator is removed). The only reason we even document the properties in the Node class is for autocomplete support in the IDE. As you see in the weird hack below in init(), we're then unsetting them because we don't actually use them but are going through __get()/__set(). If we switch to an interface (doesn't matter if it's EntityInterface or NodeInterface unfortunately because interfaces can't have properties) that is pretty much useless and we could just as well remove it. If we can keep it, maybe there is a way to use the actual class there, e.g. IntegerItem, StringItem, FileItem. Note that you can always use dpm($node->getPropertyDefinitions()) to learn about what properties an entity has. The advantage there is that it contains also dynamic properties, e.g. fields and stuff added by other modules.

- About the test, huh, didn't we change that to "Contains \Drupal\..."? :)

Berdir’s picture

Status: Needs work » Needs review

#184: node-ng-1818556-184.patch queued for re-testing.

Berdir’s picture

Issue tags: -needs profiling

Ok, did some tests with the frontpage and 10 nodes.

It is 25% slower right now for those 10 nodes. A considerable amount of that is the BC decorator that has tons of __get() and language() calls on EntityNG. I think it's therefore not that useful to do a lot of profiling now, we need to convert everything, throw out BC decorator and then do some serious profiling and see what we can optimize. It's also not really useful to try and optimize the BC decorator IMHO. We just need to get rid of it asap.

Also checked with 20 nodes and it gets slower but I think that is because the generic part makes up a smaller percentage of the whole page request.

das-peter’s picture

Fixed doc block:

+/**
+ * @file
+ * Definition of \Drupal\system\Tests\Entity\EntityBCDecoratorTest.
fago’s picture

Sure, all the usage of the BC-decorator slows down things definitely as it is decorating every get/set call. That needs to go away before release anyway though. Then, there is a remaining performance impact which we need to work on - as discussed at the comments conversion. The follow-ups here are:
#1869562: Avoid instantiating EntityNG field value objects by default
#1869574: Support single valued Entity fields
I agree with berdir that once we've removed the BC-decorator we can concentrate on performance and optimize things in a second step. However, from the comment performance optimization we already know that we want/need to do #1869562: Avoid instantiating EntityNG field value objects by default, so that can be already worked on. In fact, it's the next issue on my todo-list as it involves some changes we want to earlier rather than later.

fago’s picture

If we can keep it, maybe there is a way to use the actual class there, e.g. IntegerItem, StringItem, FileItem.

I agree with that, however it would have to be StringItem[] or so. Problem here is that we do not have a coding standard for denoting an "array" of classed objects yet, so we need to sort that out first.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good and I think we've got all questions addressed, so setting back to RTBC for re-consideration.

webchick’s picture

Title: Convert nodes to the new Entity Field API » Change notice: Convert nodes to the new Entity Field API
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Ok thanks for checking. 25% is :( :( :( However, Berdir pointed out that in our big soul-sucking critical performance issue at #1744302: [meta] Resolve known performance regressions in Drupal 8 Entity Field API is already mentioned, and #1762258: Benchmark: Bypass magic to speed up the new entity field API is cross-linked there. It makes sense that large-scale performance work is going to have to wait until after we drop the BC library. Unfortunately, catch isn't around to validate that this sort of regression is acceptable, but since this blocks 50,000 things and the performance regressions seem to be already documented, I'd like to fast-track it in. I'm obviously happy to be told it needs to be rolled back if that's out of line, though.

Committed and pushed to 8.x. Thanks!

This will need a change notice, methinks. :P

plach’s picture

Title: Change notice: Convert nodes to the new Entity Field API » Convert nodes to the new Entity Field API
Status: Active » Fixed

I updated http://drupal.org/node/1806650 to mention this issue and the comment conversion one.

fago’s picture

fago’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue tags: -Needs change record

I added a followup section to the summary, and removed the needs change notice tag

Gábor Hojtsy’s picture

Thanks for the heroic work. We now need to move on to #1498674: Refactor node properties to multilingual for D8MI so we can get rid of the node-copy translation module sooner than later and work on an upgrade path. Lots of work still dependent on this to be fixed before API freeze. Any help is welcome!

Gábor Hojtsy’s picture

Issue tags: -sprint

Moving off of D8MI sprint.

Status: Fixed » Closed (fixed)

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

sun’s picture

Status: Closed (fixed) » Needs work
+++ b/core/modules/forum/forum.module

 function forum_field_storage_pre_insert(EntityInterface $entity, &$skip_fields) {
   if ($entity->entityType() == 'node' && $entity->status && _forum_node_check_node_type($entity)) {
     $query = db_insert('forum_index')->fields(array('nid', 'title', 'tid', 'sticky', 'created', 'comment_count', 'last_comment_timestamp'));
-    foreach ($entity->taxonomy_forums as $language) {
-      foreach ($language as $item) {
-        $query->values(array(
-          'nid' => $entity->nid,
-          'title' => $entity->title,
-          'tid' => $item['tid'],
-          'sticky' => $entity->sticky,
-          'created' => $entity->created,
-          'comment_count' => 0,
-          'last_comment_timestamp' => $entity->created,
-        ));
-      }
+    foreach ($entity->getTranslationLanguages() as $langcode => $language) {
+      $translation = $entity->getTranslation($langcode, FALSE);
+      $query->values(array(
+        'nid' => $entity->id(),
+        'title' => $translation->title->value,
+        'tid' => $translation->taxonomy_forums->tid,
+        'sticky' => $entity->sticky,
+        'created' => $entity->created,
+        'comment_count' => 0,
+        'last_comment_timestamp' => $entity->created,
+      ));
     }
     $query->execute();
   }

The NG + language/translation changes are fine, but:

How and why was $entity->taxonomy_forums->tid suddenly turned into a single-valued item?

The new logic inserts a single value per language.

The old logic inserted multiple values for each language.

Berdir’s picture

Status: Needs work » Closed (won't fix)

Opened #1956848: NodeNG conversion follow-up: Only first forum tid is saved into {forum_index} for that, let's not re-open huge issues like this for something that's not super-critical :)

amateescu’s picture

Status: Closed (won't fix) » Closed (fixed)

Resetting proper status :)

Gábor Hojtsy’s picture

Retroactively tagging for D8MI.

Gábor Hojtsy’s picture

Issue summary: View changes

added follow up section