Unnecessary use of generators?

In the context of HuxDiscovery, theres no purpose to it. At least any more, from initial dev.

In the context of HuxModuleHandler, they're only generated from private methods and consumed entirely within the final class. I had intended to do other optimisations for cold runs, but didnt get to it.

Ultimately I dont think it matters. However it does convey to the internal consumer that they shouldn't expect to have all values ready, and shouldnt try to pre-assign to local variables, etc.

Since its all internal it this could change at any time without affecting others.

I think it is better to remove the generators for clarity's sake, in the public methods of HuxDiscovery and in the private methods in HuxModuleHandler.
In both these places, the full array already exists, so there is no real benefit.
And because it is internal, as you say, there is no expectation of future or 3rd party implementations to need generators in these places.

On the other hand, in HuxCompilerPass::getHuxClasses() the generator is justifiable, because this way we avoid to ever collect all the classes in an array.

However it does convey to the internal consumer that they shouldn't expect to have all values ready, and shouldnt try to pre-assign to local variables, etc.

In each of these cases:

• the array or generator only has the implementations for one specific hook, so it is not going to be huge.
• the calling code needs to traverse all values.
• it is reasonable to assume that the values already exist as an array somewhere. It is currently the case, and will likely be in the future.

As a note, without this change, I see this warning in phpunit:

1) Drupal\Tests\hux\Unit\HuxDiscoveryUnitTest::testHuxDiscovery
Passing an argument of type Generator for the $haystack parameter is deprecated. Support for this will be removed in PHPUnit 10.

The code in question is this:

    $this->assertCount(1, $discovery->getHooks('test_hook'));

See https://github.com/sebastianbergmann/phpunit/issues/4567

So either we should call iterator_count() or iterator_to_array() in the test, or we let getHooks() return an array directly.

I can confirm that the merge request does indeed fix the PHPUnit warning.

Rebased locally on the latest 1.x branch. Let me know if it looks correct or a did a dumb.

Looks good for me as well.

Could we get this merged? We've had to patch Hux for this because if it's enabled while we run PHPUnit tests for a module that requires Hux, we end up with the aforementioned PHPUnit warnings.

It seems unusual to be running tests for a contrib?

Anyway, I was forced to fix the generator deprecation as a part of MR23. You can see the fix to tests change pipelines from red to green: https://git.drupalcode.org/project/hux/-/merge_requests/23/pipelines%0A

Change is in 1.3beta

I hadn’t looked at this issue for a while because I didn’t think it was causing issues. I still think it may be a reject

It seems unusual to be running tests for a contrib?

It's not that we're not running Hux's tests - it's that our own PHPUnit tests for our own modules were raising these warnings since they use Hux and so had to have Hux installed for some tests. The generators are/were in HuxDiscovery and HuxModuleHandler which is probably why.

@Ambient.Impact do you have a error printout + trace?

I was going by the error in #4, which should only appear if executing the Hux tests.

is the error still present after 1.3 upgrade?

I just re-ran all our PHPUnit tests with 1.3 after removing the patches we'd been using and everything passes with no warnings. Not sure if it's 1.3 that fixed it or I was doing something wrong and we didn't actually need the generators patch - wouldn't be the first time I was doing something dumb - so it looks like this is no longer an issue for us.