Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.During its unit test the PHP Backend prepareItem method is called without any parameters. The PHP docs indicate the first argument is required. Either code or docs need updating.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | interdiff.txt | 937 bytes | Aki Tendo |
| #5 | 2514712-5.diff | 1.29 KB | Aki Tendo |
| #5 | failing-test.diff | 1.24 KB | Aki Tendo |
| failing-test.diff | 776 bytes | Aki Tendo | |
Comments
Comment #1
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedComment #3
catchIt's getting explicitly passed FALSE - which is the result of @include when there is no file.
getByHash() should probably check for !empty() instead of isset() - but looks like PHP is forgiving the isset($cache->data) check in prepareItem().
Comment #4
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedWhich course of action should be taken? Fix getByHash()? Revise prepareItem() 's documentation so that FALSE is a legal argument, maybe even the default? My inclination is the former - a call to prepareItem() implies an expectation that you have an item to prepare.
Comment #5
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedNow that the assert tools are core this failing test has been re-rolled.
I would like to begin tagging this issue and all others like it with "Assertion Failure" for a quicker lookup of these issues collectively.
Note that the number of failures have lowered since the last test from 41 to 26, which indicates the real source of this bugbear might not be the caller but a failure to write the cache in the first place.
Nevertheless, I've attached a solution following up on catch's comments at #3 and this removes all the failures so far detected.
IMPORTANT NOTE: Until the testbots are fixed the test case must change the .htaccess file to turn assertions on. Provided it passes tests and is RTBC the .htaccess setting change must be reverted before final commit.
Comment #6
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedComment #8
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThe solution diff passed so ready for RBTC
Comment #9
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedChanged issue tag to the one Alex Pott recommends be used for these.
Comment #10
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedBump. Still needing a code review - should be simple enough.
Comment #12
Aki Tendo CreditAttribution: Aki Tendo as a volunteer commentedThis has slid through the cracks. When last left off this was ready for final review.
Comment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedMoving back to the front of the list.
Comment #25
smustgrave CreditAttribution: smustgrave at Mobomo commentedSending to PNMI after 7 years. Can you please verify if this issue is still relevant in 9.5? I see it was originally written for D7 and tons of things have changed since.
Please reopen if still relevant with an updated issue summary. (possible reroll)
Comment #26
smustgrave CreditAttribution: smustgrave at Mobomo commentedSince this moved to PNMI over 3 months ago, which is the threshold going to close as outdated.
If still a valid issue please reopen with an updated issue summary
Thanks