- Issue created by @AndyF
- 🇬🇧United Kingdom AndyF
I don't know the module or pharborist at all, but I'm expecting to have a D7 upgrade project soon and thought I'd try and help push things forward a little.
The test error seems to be legit, I get it locally.
1) Drupal\Tests\drupalmoduleupgrader\Unit\Plugin\DMU\Fixer\DeleteTest::test Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ '<?php\n \n -' +function foo_permission() {\n + return array(\n + 'bazify' => array(\n + 'title' => 'Do snazzy bazzy things',\n + ),\n + );\n +}' /builds/issue/drupalmoduleupgrader-3465500/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:95 /builds/issue/drupalmoduleupgrader-3465500/tests/src/Unit/Plugin/DMU/Fixer/DeleteTest.php:50
As far as I can tell it's down to
\Drupal\drupalmoduleupgrader\Plugin\DMU\Fixer\Delete::execute()
calling\Drupal\drupalmoduleupgrader\Plugin\DMU\Fixer\NodeCollectorTrait::getObjects()
and getting back a\Pharborist\Node
rather than a\Pharborist\NodeCollection
. That in turn seems to be down to\Drupal\drupalmoduleupgrader\IndexerInterface::get()
returning aNodeCollection
with some indexers and an individualNode
with others (at least with\Drupal\drupalmoduleupgrader\Plugin\DMU\Indexer\Functions::get()
).All usages of
getObjects()
immediately iterate the result, so it feels like it could be corrected by ensuring we return aNodeCollection
. However I don't see how it was ever working, I don't see any obvious changes in the affected code that caused it to start failing, and if I revert the module back to 1.9 (and use the version ofjcnventura/pharborist
from the same date) I still get the same failure, so I'm nervous I'm missing something here. Also annoyingly I don't know how to get the module to use the delete plugin outside of a test, so I can't do a manual functional test and verify there definitely is an issue with the delete plugin, and it's fixed with the change. I don't see any issues in the queue that might be related. - Status changed to Needs review
4 months ago 10:24am 4 August 2024 - 🇬🇧United Kingdom AndyF
I've added the possible solution suggested in the previous comment of ensuring we always return a node collection; as mentioned before I don't know how to test it properly with the module and I'm new to it, so careful review appreciated, thanks!