Problem/Motivation

Depending on the state of the field, and which methods have been called, the return value of getValue() will be different.

Scenario

$field = User::load(1)->get('pass');
$value1 = $field->getValue();
$field->getProperties();
$value2 = $field->getValue();

Expected

$value1 === $value2

Actual

$value1 !== $value2

Proposed resolution

Switch from looping over $this->properties to $this->getProperties()

Remaining tasks

User interface changes

API changes

Data model changes

\Drupal\Core\TypedData\Plugin\DataType\Map::getValue()

CommentFileSizeAuthor
#28 2934192-28-check-test-coverage.patch1.94 KBamateescu
#15 interdiff_13-15.txt511 bytesvsujeetkumar
#15 2934192_15.patch10.58 KBvsujeetkumar
#13 2934192-rerolled-13.patch10.37 KBVidushi Mehta
#2 2934192-properties-2.patch14.34 KBtim.plunkett

Issue fork drupal-2934192

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new 2934192-properties-2.patch14.34 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2934192-properties-2.patch, failed testing. View results

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I'm not sure at all that this change is correct or that it is exposing bugs, more likey it is *causing* at least some of those test fails.

Some, maybe all places use $this->properties for two reasons as far as I know/remember:

a) An optimization, a property that was not initialized as a property object doesn't have an updated value, so we don't need to get it.
b) getProperties() does not return computed properties by default but I'm sure at least some of those calls rely on processing computed properties, things like updating them specifically need to reset computed properties, so that for example the processed text is recalculated if the value or the filter format changes.

I need to better understand what exactly the validation problem is with the other issue before I can further comment on it. I'm not saying there's nothing wrong but I'm quite sure that this is not the correct fix :)

eclipsegc’s picture

eclipsegc commented

The issue is we have a user (say... user 1) and when that user is validated by the typed data system, it loads the "unchanged entity" from the database. So now we have two copies of the same entity, but user1->pass and user2->pass don't have the same properties on them because getProperties() is never called against the unchanged entity which results in the validator's comparisons failing on password (even though they both have the same password value).

Eclipse

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

\Drupal\user\Plugin\Validation\Constraint\ProtectedUserFieldConstraintValidator::validate()
line 86:

        $changed = $items->getValue() != $account_unchanged->get($field->getName())->getValue();

$items->getValue() is

[
  'value' => 'qsdfasdfasdf',
  'existing' => NULL,
  'pre_hashed' => NULL,
],

$account_unchanged->get($field->getName())->getValue() is

[
  'value' => 'qsdfasdfasdf',
],

While the password value is identical, the comparison fails because the two other properties of \Drupal\Core\Field\Plugin\Field\FieldType\PasswordItem are not present in one of the arrays.

The original change to getValue (without the others to setValue, __clone, etc) fixed that bug.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.

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.

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs reroll

Fresh list of modules to add filter dependency in #2413471-51: FieldItemBase must check property definitions instead of already instantiated properties

Vidushi Mehta’s picture

Vidushi Mehta commented
Status: Needs work » Needs review
StatusFileSize
new 2934192-rerolled-13.patch10.37 KB

Patch rerolled.

Status: Needs review » Needs work

The last submitted patch, 13: 2934192-rerolled-13.patch, failed testing. View results

vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2934192_15.patch10.58 KB
new interdiff_13-15.txt511 bytes

Fixing Tests, Please review.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Fix comparison in ProtectedUserFieldConstraintValidator is related to #6

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bradjones1’s picture

bradjones1
Primary language English
commented

I got bit by this today, via symptoms of #3123696: Fix comparison in ProtectedUserFieldConstraintValidator (agree, that may be a duplicate.)

This is a bit dated so I'll take a stab at a "re-roll" into an MR and see how it fares against 9.5.x.

bradjones1 opened merge request !2832

bradjones1’s picture

bradjones1
Primary language English
commented

Transferred patches into an MR rebased on 9.5.x. Notable to anyone reviewing the history here, none of the "re-rolls" following the actual patch in #2 contained... the patch. So this seeks to consolidate everything here, since a lot of this requires test updates per #4.

bradjones1’s picture

bradjones1
Primary language English
commented

https://dispatcher.drupalci.org/job/drupal_patches/151438/testReport/jun...

bradjones1’s picture

bradjones1
Primary language English
commented
Status: Needs review » Needs work
bradjones1’s picture

bradjones1
Primary language English
commented

So I generally concur with Berdir in #2

I'm not sure at all that this change is correct or that it is exposing bugs, more likey it is *causing* at least some of those test fails.

and got caught up in the symptoms of this issue today. I consider myself a pretty good Drupaller and I will totally concede that even after spending hours on this today, I still can't tell you the difference between "values" and "properties."

So I'm going to step back, however if you are coming to this from #3123696: Fix comparison in ProtectedUserFieldConstraintValidator I just posted an MR there to use FieldItemList::equals() instead of PHP array comparison, which might very well be the correct answer for anyone running into this bug. That method contains much of the logic involved in working around this issue altogether and in a more eloquent way.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

amateescu’s picture

amateescu commented
StatusFileSize
new 2934192-28-check-test-coverage.patch1.94 KB

I'm curious to see what are the failing tests mentioned in those @todos.

bradjones1’s picture

bradjones1
Primary language English
commented

LOL... nothing?

amateescu’s picture

amateescu commented
Component: typed data system » field system
Status: Needs work » Needs review

This was most likely fixed by #2981889: Performance Degradation in Layout Builder and other places likely , so what's left to do here is clean up that dead code.

As for the initial scope of the issue, as @bradjones1 mentioned above in #26, if you need to compare field item objects you have to use their equals() method.

smustgrave’s picture

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

Can the issue summary be updated to include what's needed now. If it's cleaning up dead code if that could be documented please

Thanks.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.