Problem/Motivation

\Drupal\Core\Field\Plugin\Field\FieldType\PasswordItem::preSave() can result in NULL values being passed to trim() which results in a deprecation error in PHP 8.1.

Steps to reproduce

See tests in #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)

Proposed resolution

Refactor code so this does not occur.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#14 3238227-14.patch7.43 KBalexpott
#14 9-14-interdiff.txt1.61 KBalexpott
#9 3238227-9.patch7.57 KBalexpott
#9 3238227-9.test-only.patch6.07 KBalexpott
#9 7-9-interdiff.txt1.46 KBalexpott
#7 3238227-7.patch9.26 KBalexpott
#7 3238227-7.test-only.patch7.77 KBalexpott
#7 6-7-interdiff.txt535 bytesalexpott
#6 3238227-6.patch9.24 KBalexpott
#6 5-6-interdiff.txt662 bytesalexpott
#5 3238227-5.patch9.2 KBalexpott
#5 3238227-5.test-only.patch7.71 KBalexpott
#2 3238227-2.patch1.49 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3238227-2.patch1.49 KB

Here's the fix that is in [ #3220021] - I think maybe we can refactor a little bit more for clarity.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

This makes the logic easier to follow anyway, so +1 from me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I'm working on an explicit test because we don't have any test coverage. I agree that the logic is slightly better but I want a unit test to ensure we've not changed behaviour in a way that affects security.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 3238227-5.test-only.patch7.71 KB
new 3238227-5.patch9.2 KB

Here's a test that covers most of \Drupal\Core\Field\Plugin\Field\FieldType\PasswordItem::preSave() - found a bug when saving a password that trims to an empty string. This doesn't really affect the UI because the UI trims the password input but it does affect JSON:API - so will file a follow-up issue to address this.

The test only patch should pass to show that the behaviour has not changed.

In order to test the field type we need to fix the fact that the config schema is missing for the field type.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 5-6-interdiff.txt662 bytes
new 3238227-6.patch9.24 KB

Fixing the @todo to link to #3238399: \Drupal\Core\Field\Plugin\Field\FieldType\PasswordItem can save an unhashed value in error

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 6-7-interdiff.txt535 bytes
new 3238227-7.test-only.patch7.77 KB
new 3238227-7.patch9.26 KB

Adding the test group :( - as before the test only patch exists to prove behaviour has not changed and is expected to pass.

Status: Needs review » Needs work

The last submitted patch, 7: 3238227-7.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 7-9-interdiff.txt1.46 KB
new 3238227-9.test-only.patch6.07 KB
new 3238227-9.patch7.57 KB

Oops didn't mean to leave the changes to core/modules/user/tests/src/Functional/Rest/UserResourceTestBase.php in. That was part of investigating #3238399: \Drupal\Core\Field\Plugin\Field\FieldType\PasswordItem can save an unhashed value in error and determining that it is an issue for jsonapi. Removing those changes here...

As before the test only patch exists to prove behaviour has not changed and is expected to pass.

andypost’s picture

andypost
he/him
Primary language Russian
commented

test-only passes

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@andypost test-only is supposed to pass. See earlier comments. The test is being added to prove that behaviour is not changed. The fact we have to add schema here proves we are lacking test coverage of using PasswordItem as a configurable field. All of the current test coverage of this code is on the User entity level and not the field type level.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#2227381: Apply formatters and widgets to User base fields 'name' and 'email'

Sorry, missed second part of comment

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/Drupal/KernelTests/Core/Field/FieldType/PasswordItemTest.php
@@ -0,0 +1,194 @@
+
+    // Add a datetime range field.
+    $this->fieldStorage = FieldStorageConfig::create([
+      'field_name' => 'test_field',
+      'entity_type' => 'entity_test',
+      'type' => 'password',
+    ]);
+    $this->fieldStorage->save();

This isn't a datetime range field.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 9-14-interdiff.txt1.61 KB
new 3238227-14.patch7.43 KB

Indeed it is not. That comment and others in this test are not really necessary. They're not adding to the code. Setting back to rtbc as this is only removing comments.

  • catch committed b6e74cf on 9.3.x 
    Issue #3238227 by alexpott: \Drupal\Core\Field\Plugin\Field\FieldType\...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed b6e74cf and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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