- Issue created by @darkodev
- First commit to issue fork.
- Assigned to bharath-kondeti
- last update
about 1 year ago 720 pass - last update
about 1 year ago 720 pass - Status changed to Needs review
about 1 year ago 6:39am 5 June 2024 - Status changed to Needs work
about 1 year ago 7:02am 5 June 2024 - ๐ณ๐ฑ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.
- last update
about 1 year ago 720 pass - Status changed to Needs review
about 1 year ago 12:35pm 5 June 2024 - Status changed to Needs work
about 1 year ago 1:01pm 5 June 2024 - ๐ณ๐ฑ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. - last update
about 1 year ago 719 pass, 3 fail - Status changed to Needs review
about 1 year ago 7:38am 10 June 2024 - ๐ฎ๐ณIndia bharath-kondeti Hyderabad
@MegaChriz Tried to fix the review. Please check and do let me know, if anymore changes are needed.
- Status changed to Needs work
about 1 year ago 8:17am 10 June 2024 - ๐ณ๐ฑNetherlands megachriz
- I think that
property_exists()
should be used in bothBaseItem::get()
andBaseItem::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.
- I think that
- last update
about 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.
- ๐ฎ๐ณIndia ankitv18
Reviewed the MR!174 and suggested few changes
cc: @MegaChriz
- last update
about 1 year ago 721 pass, 2 fail - last update
about 1 year ago 719 pass, 3 fail - last update
about 1 year ago 719 pass, 3 fail - last update
about 1 year ago 719 pass, 3 fail - last update
about 1 year ago 719 pass, 3 fail - last update
about 1 year ago 719 pass, 3 fail - First commit to issue fork.
- ๐บ๐ธ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.
- ๐ณ๐ฑ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.
- ๐บ๐ธ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.
- Status changed to Needs review
about 1 month ago 2:00pm 18 May 2025 -
megachriz โ
committed f27462fd on 8.x-3.x authored by
immaculatexavier โ
Issue #3452563 by megachriz, bharath-kondeti, ankitv18, bwoods, baikho:...
-
megachriz โ
committed f27462fd on 8.x-3.x authored by
immaculatexavier โ
- ๐ณ๐ฑ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).