Problem/Motivation

In Drupal Commerce, we created a field that storage a plugin's ID and configuration. The latter is saved as a serialized blob (https://github.com/drupalcommerce/commerce/blob/8.x-2.x/src/Plugin/Field...).

When the field is attached to an entity type and has a dedicated table the target_plugin_configuration property is unserialized just fine. However, if the field is attached as a base field item and part of the shared table, it does not.

You can see that \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromDedicatedTables takes into consideration column schema being marked as serialized. And then \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromSharedTables does not, at all.

Proposed resolution

Improve \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromSharedTables to handle serialized column data.

Remaining tasks

User interface changes

None.

API changes

Serialized field properties of base fields are now deserialized automatically before being passed to field types and they no longer need to support them as a string.

That means all field types must support that their values are passed as a a deserialized data structure. If a field type was only used as a base field then it might not be supporting that properly.

Data model changes

None.

Release notes snippet

Serialized properties of base fields are now automatically unserialized to be consistent with configurable fields. Existing workarounds for this bug might need to be adjusted if they relied the old behavior of passing a string to those fields.

CommentFileSizeAuthor
#62 values_in_shared_table-2788637-62-interdiff.txt1.29 KBBerdir
#62 values_in_shared_table-2788637-62.patch7.83 KBBerdir
#57 values_in_shared_table-2788637-57-interdiff.txt4.17 KBBerdir
#57 values_in_shared_table-2788637-57.patch7.82 KBBerdir
#53 values_in_shared_table-2788637-53-interdiff.txt2.01 KBBerdir
#53 values_in_shared_table-2788637-53.patch8.23 KBBerdir
#51 values_in_shared_table-2788637-50.patch8.15 KBBerdir
#50 values_in_shared_table-2788637-50.patch6.91 KBbradjones1
#48 values_in_shared_table-2788637-39-42-interdiff.txt1.77 KBBerdir
#42 values_in_shared_table-2788637-42.patch5.93 KBbradjones1
#42 interdiff.txt4.59 KBbradjones1
#41 values_in_shared_table-2788637-41.patch5.97 KBbradjones1
#41 interdiff.txt4.63 KBbradjones1
#39 values_in_shared_table-2788637-39.patch4.46 KBeffulgentsia
#26 values_in_shared_table-2788637-26.patch4.46 KBmglaman
#21 values_in_shared_table-2788637-21.patch3.59 KBmglaman
#21 interdiff-2788637-21-19.txt685 bytesmglaman
#19 values_in_shared_table-2788637-19.patch3.32 KBmglaman
#12 values_in_shared_table_PASS-2788637-12.patch2.7 KBmglaman
#12 values_in_shared_table_FAIL-2788637-12.patch763 bytesmglaman
#6 values_in_shared_table-2788637-6.patch1.95 KBmglaman
#3 values_in_shared_table-2788637-3.patch1.95 KBmglaman
SqlContentEntityStorage_php_-_commerce2x_-____Drupal_sites_commerce2x_.png159.27 KBmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mglaman created an issue. See original summary.

mglaman’s picture

For instance, hacking the code for a quick test, the following fixes our tests

if ($property_name == 'target_plugin_configuration') {
  $values[$id][$field_name][$langcode][$property_name] = unserialize($values[$id][$field_name][$langcode][$property_name]);
}
mglaman’s picture

Here is the patch. Might be simplified if we could access $fieldStorageDefinitions inside of \Drupal\Core\Entity\Sql\DefaultTableMapping

Adds same logic as found in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromDedicatedTables for checked schema marked as serialized.

mglaman’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: values_in_shared_table-2788637-3.patch, failed testing.

mglaman’s picture

Status: Needs work » Needs review
FileSize
1.95 KB

Made patch against outdated local checkout. Re-roll.

borisson_’s picture

I think this is related with #2788023: Serialized single-value base field columns are not unserialized when loading entities. Even though it's for different types of storage.

Do you think it's possible to fix both issues at the same time?

mglaman’s picture

Thanks borisson_, it's the same issue. Linking the two, we need a maintainer to decide proper approach. This one follows what dedicated table does.

mglaman’s picture

Marking for subsystem maintainer review so we know which approach to take, this or linked issue in #8

mglaman’s picture

FWIW went to write a core test, and the MapItem FieldType works fine because it handles unserialization itself, and all that. Experiencing this with a field which has multiple columns, one or more of which is serialized. Which core does not have a field like this to test against.

mglaman’s picture

Assigned: Unassigned » mglaman

Discussed with berdir in IRC. Core does has a test case: http://cgit.drupalcode.org/drupal/tree/core/modules/link/src/Plugin/Fiel...

I need to upload a patch with the unserialize workaround removed to prove failure. Then with removal and my fix.

mglaman’s picture

Ok. Here's a patch which should fail by removing unserialize check in LinkItem. Then a passing patch with unserialize fix.

The last submitted patch, 12: values_in_shared_table_FAIL-2788637-12.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: values_in_shared_table_PASS-2788637-12.patch, failed testing.

The last submitted patch, 12: values_in_shared_table_FAIL-2788637-12.patch, failed testing.

The last submitted patch, 12: values_in_shared_table_PASS-2788637-12.patch, failed testing.

The last submitted patch, 12: values_in_shared_table_FAIL-2788637-12.patch, failed testing.

The last submitted patch, 12: values_in_shared_table_PASS-2788637-12.patch, failed testing.

mglaman’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

Fix link item test that assumed data had to be serialized.

diff --git a/core/modules/link/tests/src/Kernel/LinkItemTest.php b/core/modules/link/tests/src/Kernel/LinkItemTest.php
index 5e2c45d..c39f2be 100644
--- a/core/modules/link/tests/src/Kernel/LinkItemTest.php
+++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
@@ -142,7 +142,7 @@ public function testLinkItem() {
     // properly initialized.
     $entity->field_test = [
       'uri' => 'internal:/node/add',
-      'options' => serialize(['query' => NULL]),
+      'options' => ['query' => NULL],
     ];
     $this->assertEqual($entity->field_test->uri, 'internal:/node/add');
     $this->assertNull($entity->field_test->title);

Berdir’s picture

+++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
@@ -142,7 +142,7 @@ public function testLinkItem() {
     // properly initialized.
     $entity->field_test = [
       'uri' => 'internal:/node/add',
-      'options' => serialize(['query' => NULL]),
+      'options' => ['query' => NULL],
     ];

the comment above this should be updated too, not visible here but it actually explicitly talks about serialized.

mglaman’s picture

Updated the comment for the test, per #20

Berdir’s picture

We plan to add a test field type that we can use (we need something there that actually has an *object* as the value to be serialized) in #2609252: Boolean field with #access FALSE cause EntityStorageException. That will add another @todo to this issue and once it is in, we can update that and should have sufficient/explicit/dedicated/documented test coverage to get this in.

The implementation makes sense to me and is consistent with loadFromDedicatedTables(), the only question I guess is if we could share the code between the two, but the rule of three states that you should only try to share code when you have the third implementation of something :)

mglaman’s picture

I don't see much of a gain by sharing the code between load shared or load dedicated. I'm pretty sure I had tried working on that. Or the method was so minimal (like an is serialized/serializable check.)

Berdir’s picture

Status: Needs review » Needs work

Yeah, agreed.

The other issue landed, so lets remove the @todo in \Drupal\field_test\Plugin\Field\FieldType\TestObjectItem::setValue() and I think we're good to go.

mglaman’s picture

Assigned: Unassigned » mglaman

Working on this

mglaman’s picture

Assigned: mglaman » Unassigned
Status: Needs work » Needs review
FileSize
4.46 KB

Update patch which removes the @todo and tests the original patch!

Berdir’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
@@ -138,11 +138,11 @@ public function testLinkItem() {
 
-    // Check that if set uri and serialize options then the default values are
+    // Check that if we set uri and options then the default values are
     // properly initialized.
     $entity->field_test = [
       'uri' => 'internal:/node/add',
-      'options' => serialize(['query' => NULL]),
+      'options' => ['query' => NULL],
     ];

One could argue that removing support for the string input is an API change for existing code.

I think it never was an API and hope people didn't do stuff like this, this was kind of a test for that code that we no longer have.

If core committers disagree, we could keep it since we have a test field type now that provides sufficient test coverage.

Marking as RTBC. A test-fail patch wouldn't hurt and maybe we should do a CR to notify people that this is now working and they can rely on it/remove their hacks.

We've marked major issues as duplicate, so I think this should be major as well.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +Needs release manager review, +Needs change record

I think this is an important bug fix since the system is very much not behaving as one would expect. But the fix introduces a nasty BC issue where code that was previously unserialising will now not need to. So at the very least the we need a CR to announce this and we probably need to only do this in 8.3.x and the release managers need to have a think about BC too.

The API section of the issue summary needs updating because I think we have two API changes here - one on setting values and one on retrieving values.

Berdir’s picture

I'm fine with a change record but I think the change is not as problematic as you think:

This issue fixes the missing unserialize for base fields/shared tables, it already works fine for configurable fields/dedicated tables. That means field types already need to handle both cases. And even if it is a special field type only used for a certain base field in a custom entity type, it already needs to be dynamic to support setting the value through the API/REST (normalize/denormalize) and so on.

I'm reasonably sure that this will not break any existing field types, we can just tell them that they can remove this hack now.

Also, serializing of both base and configurable fields already works fine and it only affects calling setValue() (directly or indirectly). Accessing it with getValue() will not change here unless a field type wouldn't have the unserialize logic but then he would be broken right now.

The biggest BC problem that I see is actually custom code that would have set a serialized string on the LinkItem, like one of our tests did. And as commented, we could just keep that logic to avoid that BC break. But I really hope nobody did that except us to test the code.

alexpott’s picture

+++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -186,13 +186,6 @@ public function setValue($values, $notify = TRUE) {
-    // Unserialize the values.
-    // @todo The storage controller should take care of this, see
-    //   SqlContentEntityStorage::loadFieldItems, see
-    //   https://www.drupal.org/node/2414835
-    if (is_string($values['options'])) {
-      $values['options'] = unserialize($values['options']);
-    }

Actually I don't get why we're changing this here - isn't it kinda out-of-scope. It's not necessary for the changes in SqlContentEntityStorage which are the real meat of this change right? If we don't do this then there are no API changes that field types don't already need to deal with right? So then this change could be eligible for 8.2.x.

So lets keep the code to avoid the BC break and open a new issue to address that specific issue.

Berdir’s picture

Yes, it is not required for that change to work, but it is also dead code now that we no longer need (in core at least). Before we had the test field type, it was test coverage that it is no longer required.

We don't need it anymore for test coverage, it's just dead code now. If we don't remove it here, we need another follow-up issue and update the @todo. IMHO, it would be fine to remove it but as I wrote in #27, if committers prefer to keep it, fine with me.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pwolanin’s picture

Are all these serialized columns simple data like associative arrays? Would we be better off using JSON as a serialization format?

pwolanin’s picture

What remains here to get back to RTBC? This has been waiting a while now.

mglaman’s picture

Are all these serialized columns simple data like associative arrays? Would we be better off using JSON as a serialization format?

I don't see benefit here versus normal serialization. Especially if it's more than just simple data (never assume)

What remains here to get back to RTBC? This has been waiting a while now.

No idea. Someone to say "Nah, this isn't BC, or internal enough to be three sheets to the wind about it." But I don't dive into that level of bikeshed in core. I don't see it as BC, never did. Saw it as a bug, and a bugfix this deep shouldn't be BC.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

effulgentsia’s picture

Version: 8.5.x-dev » 8.6.x-dev
Status: Needs work » Needs review
FileSize
4.46 KB

#26 no longer applies. This is a straight reroll, containing only changes to context lines, not to any code being added or removed.

bradjones1’s picture

I'm not sure if this is in scope on this issue or belongs elsewhere, but I would add that this logic is also totally unreachable if $this->dataTable is NULL on the storage. In my case, that's a result of the entity type being neither revisionable nor translatable, and as such the property is nullified and then never set in ::initTableLayout().

This particular code is pretty old, dating to the earliest ports in D8, so I wonder if this is an oversight or purposeful and I'm just not familiar enough with the subsystem to understand the motivation.

https://github.com/drupal/drupal/blame/8.6.x/core/lib/Drupal/Core/Entity...

bradjones1’s picture

It actually occurred to me after looking at this more closely that my edge case requires a grafting of this new logic in a third place in the class when values are processed directly off the table; see the attached patch and interdiff.

bradjones1’s picture

Added an extra conditional onto the single-property unserialization rule, as apparently some columns in the DB do not map to a storage definition. Interdiff is still against #39.

The last submitted patch, 41: values_in_shared_table-2788637-41.patch, failed testing. View results

borisson_’s picture

I'm not sure if the changes in #41/#42 should go in a new issue and discussed there, or if we can implement them here. I added a start for a change record, by using a lot of words from @Berdir in #29.

Berdir’s picture

The interdiff in the last two patches are very confusing, any change you could provide on between your latest patch and the one before yours?

+++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -186,13 +186,6 @@ class LinkItem extends FieldItemBase implements LinkItemInterface {
     }
-    // Unserialize the values.
-    // @todo The storage controller should take care of this, see
-    //   SqlContentEntityStorage::loadFieldItems, see
-    //   https://www.drupal.org/node/2414835
-    if (is_string($values['options'])) {
-      $values['options'] = unserialize($values['options']);
-    }
     parent::setValue($values, $notify);

Lets keep this, but put a @deprecated in there, saying that passing in a serialized string value property is deprecated in Drupal 8.6.0 and will no longer be supported 9.0.

That still gives us test coverage for not doing it in core as without adding the skipped deprecation message, tests will fail, but we don't break contrib/custom.

It's fine to keep it removed in the test field type.

Berdir’s picture

Status: Needs review » Needs work
bradjones1’s picture

@Berdir - The interdiff is against the patch in #39, so I think I've provided what you asked for? My addition is just to include the same logic as the original patch but also for base fields stored directly in the main entity data table. Let me know what else you need.

Berdir’s picture

I'm not sure what went wrong with yours exactly, but both your interdiffs are pretty large and are shifting around a ton of code.

I applied #39 and your latest patch two different branches and diffed them, and the result was the attached interdiff, which does actually make a lot more sense. It's 1.8kb instead of your 4.6kb files.

About your question in #40, it certainly is the same problem, the thing is that we don't have any field type like that in core (that has a single serialized property), so we would need to add that, an entity type that uses it and a test that saves/loads such an entity.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bradjones1’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.15 KB

Rebased, keeping the options-as-string support but adding a @trigger_error() to mark it as deprecated.

Wim Leers’s picture

This was RTBC a long time ago. What do you think still needs to happen before this can be RTBC again, @Berdir?

Berdir’s picture

Dealing with the Needs tags I suppose, they were added in #28, and I think release manager review is no longer necessary now that we are properly deprecating passing it in as a string. I've also updated the API changes section of the issue summary

There is already a change record, but I guess the wording needs to be updated a bit: https://www.drupal.org/node/2961643. I'll give that a go.

Updating the patch to reference that.

Berdir’s picture

CR is now updated.

amateescu’s picture

Issue tags: -Needs change record

This looks good to me overall. Here's a review:

  1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -434,11 +435,16 @@ protected function mapFromStorageRecords(array $records, $load_from_revision = F
    +        $storage_definition = $storage_definitions[$field_name];
    ...
    +          $definition_columns = $storage_definition->getColumns();
    

    Can we skip the assignment of $storage_definition and just do $columns = $storage_definitions[$field_name]->getColumns();.

    This would also match the code from the else block below.

  2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -447,7 +453,17 @@ protected function mapFromStorageRecords(array $records, $load_from_revision = F
    +            if (!empty($storage_definitions[$field_name])) {
    

    This if is not needed :)

  3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -552,14 +569,19 @@ protected function loadFromSharedTables(array &$values, array &$translations, $l
    +          $storage_definition = $storage_definitions[$field_name];
    +          $definition_columns = $storage_definition->getColumns();
    

    Same here, let's skip the first assignment.

  4. +++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
    @@ -138,11 +138,11 @@ public function testLinkItem() {
    +    // Check that if we set uri and options then the default values are
         // properly initialized.
    

    'properly' fits into the first line now.

  5. +++ b/core/modules/link/tests/src/Kernel/LinkItemTest.php
    @@ -174,4 +174,23 @@ public function testLinkItem() {
    +    $this->assertEqual($entity->field_test->uri, 'internal:/node/add');
    

    How about assertEquals()? :)

andypost’s picture

As already discussed in #28-29 it introduces BC break for all contrib and custom code, moreover it makes it mostly impossible to use custom serializers which were allowed at field level.
If we going to make it so - it should allow configuration of serializer or allow prevent this unserialize but storage level.
As d8 fields supposed to maintain their schema why mapping made at level upper?

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -434,11 +435,16 @@ protected function mapFromStorageRecords(array $records, $load_from_revision = F
+                  ? unserialize($record->{$column_name})

@@ -447,7 +453,17 @@ protected function mapFromStorageRecords(array $records, $load_from_revision = F
+                  ? unserialize($record->{$column_name})

@@ -552,14 +569,19 @@ protected function loadFromSharedTables(array &$values, array &$translations, $l
+            $values[$id][$field_name][$langcode] = (!empty($column_attributes['serialize'])) ? unserialize($row[$column_name]) : $row[$column_name];
...
+              $values[$id][$field_name][$langcode][$property_name] = (!empty($column_attributes['serialize'])) ? unserialize($row[$column_name]) : $row[$column_name];

+++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldType/TestObjectItem.php
@@ -46,17 +46,4 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
-      // @todo Remove this in https://www.drupal.org/node/2788637.
-      if (is_string($values['value'])) {
-        $values['value'] = unserialize($values['value']);

+++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -186,11 +186,10 @@ public function setValue($values, $notify = TRUE) {
-    // Unserialize the values.

it hardcodes php-serialization the level up (to storage) so custom or contrib code no longer can use igbinary or msgpack and no ability to make it swappable like #839444: Make serializer customizable for Cache\DatabaseBackend

Berdir’s picture

#55:

1. Yes, fixed.
2. Ah yes, it used to be necessary, but isDefaultRevision has apparently been moved into a separate loop now.
3-5. Fixed.

#56: As I said in #29, configurable/dedicated table fields already do call unserialize automatically. Only custom field types that were only used as a base field were able to rely on it being a string and even then it was a bad idea because you also forced any direct call to setValue() to pass in a serialized string. Once serialize/deserialize behavior is consistent, we can think about using something else than serialize() or making it more flexible, but setValue() is *not* the place to deal with that. Yes, there is a possible BC break and I in fact know that payment will need to be updated, and I just prepared it for that: #3035265: Make the unserialize() call in Payment::__construct() conditional. I do believe that this change makes sense and the benefits clearly outweigh the problems.

mglaman’s picture

As I have said before, this isn't a BC break. It's fixing a bug that everyone else skated around and refused to fix.

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Fix looks good, amateescu's feedback was addressed, proper deprecation added + a test.

amateescu’s picture

I agree with #57 / #58, the fact that base fields do not behave like configurable fields is a bug and it shouldn't be regarded as a "flexible extension point" at all, IMO.

As @Berdir said, this patch brings sanity in this area and allows us to start thinking about how exactly do we want to make the serialization process customizable.

RTBC +1.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: values_in_shared_table-2788637-57.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.83 KB
1.29 KB

1800 test fails due to deprecation, nice, BC layer is working ;)

Berdir’s picture

Tested that #57 still applies and passes on 8.6, but considering that this will likely cause some disruptions in contrib (for example with payment.module as mentioned above, not aware of any other cases that will break) I think that it is better to not backport this, even though it would fix some issues that people have with the security hardening we did (I think the exploit is now public enough to mention the connection publicly). Those affected by that will have to use the patch in 8.6.

alexpott’s picture

Given where we are at in the release cycle shall we commit this to 8.8.x so custom / contrib have a whole cycle to learn about and fix for this bug fix?

bojanz’s picture

Contrib has learned about this problem in the 8.2.x cycle.
The fix has been stuck and postponed ever since.

A number of Commerce installs have been affected by the recent security release (#3034976: Cannot retrieve saved objects properly from Order data field), and this issue is the actual fix.
Without it, we'll need to tell people to spend the next 6 months patching core.

All of that feels way too much for what is essentially a one-line fix.

amateescu’s picture

Agreed with @Berdir and @bojanz, I think it's much more useful to have this fix in 8.7.0-alpha1 because contrib module authors and production sites are more likely to test their codebase against an alpha release rather than the next dev branch.

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Issue tags: +8.7.0 release notes

Happy to go with what the entity API maintainers decide.

Can someone please update the release notes section of the issue summary? Thanks

Committed and pushed 54ff737a18 to 8.8.x and e866ce6c13 to 8.7.x. Thanks!

FILE: ...upal/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 480 | ERROR | [x] Expected 1 space before "?"; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Fixed on commit.

Berdir’s picture

Issue summary: View changes

  • alexpott committed 54ff737 on 8.8.x
    Issue #2788637 by mglaman, Berdir, bradjones1, effulgentsia, alexpott,...

  • alexpott committed e866ce6 on 8.7.x
    Issue #2788637 by mglaman, Berdir, bradjones1, effulgentsia, alexpott,...

Status: Fixed » Closed (fixed)

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