Creation of dynamic property Drupal\feeds\Feeds\Item\SyndicationItem::$parent:fid is deprecated

Created on 4 June 2024, over 1 year ago
Updated 5 June 2024, over 1 year ago

Problem/Motivation

When importing Feeds items via the UI, we see the following error:
Deprecated function: Creation of dynamic property Drupal\feeds\Feeds\Item\SyndicationItem::$parent:fid is deprecated in Drupal\Core\Queue\Batch->claimItem() (line 31 of /var/www/html/html/core/lib/Drupal/Core/Queue/Batch.php)

Steps to reproduce

Install latest Drupal 10.2.6 and Feeds 8.x-3.0-beta4 on PHP 8.2.
Create a Feeds type and a Feed and import it via the UI. Have not tried this via cron.

Proposed resolution

Make Feeds compatible with latest version of Core Batch API.

๐Ÿ› Bug report
Status

Active

Component

Code

Created by

๐Ÿ‡จ๐Ÿ‡ฆCanada darkodev

Live updates comments and jobs are added and updated live.
  • PHP 8.2

    The issue particularly affects sites running on PHP version 8.2.0 or later.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @darkodev
  • First commit to issue fork.
  • Assigned to bharath-kondeti
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia bharath-kondeti Hyderabad
  • Merge request !174Update file SyndicationItem.php โ†’ (Merged) created by immaculatexavier
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    720 pass
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 10.2.x + Environment: PHP 8.2 & MySQL 8
    last update over 1 year ago
    720 pass
  • Pipeline finished with Success
    over 1 year ago
    Total: 1299s
    #191408
  • Status changed to Needs review over 1 year ago
  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands megachriz

    'parent:fid' is actually a dynamic property. It is generated in \Drupal\feeds\Feeds\Source\BasicFieldSource. Besides 'parent:fid', you also have 'parent:uid', 'parent:created', 'parent:title' and many others.

    So simply declaring $parent_fid as a variable here is not a sustainable solution.

    So I think that BaseItem needs to check in methods `get()` and `set()` if the property to set exists. And if not, write to `$data` property like DynamicItem does. I don't know however if there any drawbacks in that approach, but I think the risk for that is low.

  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    720 pass
  • Pipeline finished with Success
    over 1 year ago
    Total: 1212s
    #191783
  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia bharath-kondeti Hyderabad
  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands megachriz

    Almost. BaseItem should check first if the requested field exists as property in the class. And if not, fallback to the $data array.

  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    719 pass, 3 fail
  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia bharath-kondeti Hyderabad
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia bharath-kondeti Hyderabad

    @MegaChriz Tried to fix the review. Please check and do let me know, if anymore changes are needed.

  • Pipeline finished with Failed
    over 1 year ago
    Total: 1420s
    #195330
  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands megachriz
    • I think that property_exists() should be used in both BaseItem::get() and BaseItem::set().
    • BaseItem::get() should check if $this->data[$field] exists before returning that.
    • BaseItem::toArray() should merge the object vars with $data array and then remove the extra data array from it.
    • In Syndicatiโ€ŽonItem.phpโ€Ž there is a blank line removed that should not have been removed.
    • Note that the code is currently failing tests.
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    719 pass, 3 fail
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia bharath-kondeti Hyderabad

    Reverted the blank line code on Syndicatiโ€ŽonItem.php for failing tests. I will working on the remaining.

  • Pipeline finished with Failed
    over 1 year ago
    Total: 1597s
    #195357
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ankitv18

    Reviewed the MR!174 and suggested few changes

    cc: @MegaChriz

  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    721 pass, 2 fail
  • Pipeline finished with Failed
    over 1 year ago
    Total: 1433s
    #197034
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    719 pass, 3 fail
  • Pipeline finished with Failed
    over 1 year ago
    Total: 1251s
    #197065
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    719 pass, 3 fail
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    719 pass, 3 fail
  • Pipeline finished with Canceled
    over 1 year ago
    Total: 16s
    #197300
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    719 pass, 3 fail
  • Pipeline finished with Failed
    over 1 year ago
    Total: 177s
    #197301
  • Pipeline finished with Failed
    over 1 year ago
    Total: 1149s
    #197303
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    719 pass, 3 fail
  • Pipeline finished with Failed
    over 1 year ago
    Total: 1266s
    #197320
  • First commit to issue fork.
  • Merge request !209Allow dynamic properties for feed items. โ†’ (Open) created by bkosborne
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bkosborne New Jersey, USA

    I created a new MR that just tells PHP it's OK to have dynamic properties on the item. This is a simple approach. I'm not sure if this is really the best long term solution.

  • Pipeline finished with Failed
    7 months ago
    Total: 226s
    #444179
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands megachriz

    @bkosborne
    Thanks for your contribution. This could be a viable solution too, though it is possible that it will no longer be supported in a future version of PHP (PHP 9?). I'm not sure yet though if it is going to be removed, it's not clear from https://www.php.net/manual/en/language.oop5.properties.php#language.oop5...

    Meanwhile, I've updated the other MR:

    • I've addressed my own remarks from #11.
    • I made sure that the data property cannot be get or set directly. An UnexpectedValueException will be thrown when trying to do so. Added test coverage for this change.
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium baikho Antwerp, BE

    Tested MR !174 on 3 custom Feeds Importers that extend the BaseItem class.

    • Drupal Core 11.1.3
    • PHP 8.3.15
    • Feeds v3.0.0

    Works as expected and no PHP deprecation notices now. RTBC for me

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bwoods

    Another confirmation that this patch does the trick. My specs:

    Drupal Core 10.4.6
    PHP 8.3.20
    Feeds v3.0.0

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands megachriz

    I adjusted the code a bit, because I did not like that 'data' became a preserved key. The changes ensure that a field called 'data' can be set without overwriting the complete $data property.

    I also expanded the test coverage a bit.

  • Pipeline finished with Skipped
    5 months ago
    #500184
  • Pipeline finished with Skipped
    5 months ago
    #500186
  • Status changed to Needs review 5 months ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands megachriz

    I looked through the code one more time, did not spot anything suspicious. Merged it. Thanks all who helped on this issue.

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

  • ๐Ÿ‡ช๐Ÿ‡จEcuador jwilson3

    Any chance we could get a 3.1 (or 3.0.1) release that includes this fix? This generates a lot of unnecessary noise in logs.

    We are seeing deprecation warnings on cron run after updating to PHP 8.3.21 and clearing caches. These are only notice errors, I think, so we should still be fine. But there are about 500 of them after running cron from the admin UI.

        Deprecated function: Creation of dynamic property 
        Drupal\feeds\Feeds\Item\SyndicationItem::$blog 
        is deprecated in Drupal\feeds\Feeds\Item\BaseItem->set() 
        (line 21 of modules/contrib/feeds/src/Feeds/Item/BaseItem.php).
    
        Deprecated function: Creation of dynamic property 
        Drupal\feeds\Feeds\Item\SyndicationItem::$parent:field_group 
        is deprecated in Drupal\Core\Queue\DatabaseQueue->claimItem() 
        (line 150 of core/lib/Drupal/Core/Queue/DatabaseQueue.php).
    
  • Status changed to Fixed 17 days ago
Production build 0.71.5 2024