Problem/Motivation

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

The \Drupal\aggregator\ItemsImporter::refresh() using to calculate hash of feed's source_string which could be NULL

Also no reason to calculate hash when fetcher failed to instantiate feed

Steps to reproduce

php81 -r 'var_dump(hash("sha256", NULL));'
PHP Deprecated:  hash(): Passing null to parameter #2 ($data) of type string is deprecated in Command line code on line 1

Deprecated: hash(): Passing null to parameter #2 ($data) of type string is deprecated in Command line code on line 1
string(64) "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"

Proposed resolution

calculate only in success and prevent use of NULL

Remaining tasks

review/commit

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 interdiff.txt1.43 KBandypost
#8 3239552-8.patch743 bytesandypost
#2 interdiff.txt765 bytesandypost
#8 interdiff.txt702 bytesandypost
#2 3239552-2.patch749 bytesandypost
#6 3239552-6.patch1.41 KBandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 3239552-2.patch749 bytes
new interdiff.txt765 bytes

@alexpott the interdiff vs changes from META

daffie’s picture

daffie commented 
+++ b/core/modules/aggregator/src/ItemsImporter.php
@@ -108,7 +108,7 @@ public function refresh(FeedInterface $feed) {
-    $hash = hash('sha256', $feed->source_string);
+    $hash = $success ? hash('sha256', $feed->source_string ?? '') : '';

Is this much change necessary? Maybe better to do:

  $hash = hash('sha256', $feed->source_string ?? '');
alexpott’s picture

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

@daffie doing a hash when success is false is doing unnecessary work. This change does that and stops the deprecation.

alexpott’s picture

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

Actually... the change is not correct.

The change that passes tests is $hash = $success ? hash('sha256', $feed->source_string) : ''; If success is true then $feed->source_string is going to be set.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.43 KB
new 3239552-6.patch1.41 KB

debugged it and found that source string is always string when $status is TRUE, moreover fetcher \Drupal\aggregator\Plugin\aggregator\fetcher\DefaultFetcher::fetch() overrides it to FALSE

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work 
+++ b/core/modules/aggregator/src/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -82,7 +82,7 @@ public static function create(ContainerInterface $container, array $configuratio
-    $feed->source_string = FALSE;
+    $feed->source_string = '';

This change is not necessary for PHP 8.1 compatibility. It's a subtle behaviour change that I'm not sure we should make here. I think we should open a follow-up to move this away from a magic property and to something better like a method.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Related issues: +#2095603: [meta] Complete Entity Field API
StatusFileSize
new interdiff.txt702 bytes
new 3239552-8.patch743 bytes

I think the follow-up exists #2862574: Add ability to track an entity object's dirty fields (and see if it has changed)

alexpott’s picture

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

@andypost I'm not sure #2862574: Add ability to track an entity object's dirty fields (and see if it has changed) is the follow-up here. source_string is a dynamically assigned property - it's not a field in the entity sense of the word. I think due to \Drupal\Core\Entity\ContentEntityBase::__set() we end up shoving it in the \Drupal\Core\Entity\ContentEntityBase::$values array. It's all a bit messy.

alexpott’s picture

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

This change is ready though and I don't think we need a follow-up to exist prior to committing as the whole entity::__set() thing is likely to become more of an issue in the future.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Reviewed & tested by the community » Needs review

As I see this kind of changes is tracked under #2016679: Expand Entity Type interfaces to provide methods, protect the properties

For example
- #2532776: Add the undeclared variable $in_preview to the class Comment
- #2527114: Create PHPUnit test that Entity class variables are protected or are allowed public

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

  • catch committed 51166f1 on 9.3.x 
    Issue #3239552 by andypost, alexpott, daffie: Improve hash() calculation...
catch’s picture

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

Committed 51166f1 and pushed to 9.3.x. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented

Thank you, filed follow-up #3239690: Add undeclared variables to the Feed entity class

Status: Fixed » Closed (fixed)

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