Removing a Hux hook implementation/class can cause fatal errors

Wondering if setting a deployment identifier would prevent this as we would have a fresh container?

I'd consider closing this issue if deployment identifiers solve this.

See also #3153335: Document $settings['deployment_identifier'] (and that it fixes moved class autoloader caching) →

Deployment identifiers do not solve this issue (we just tried fixing the same sort of error this way). Is it because hux doesn't use the deployment_identifier in its cache key?

Adding steps to reproduce.

Perhaps theres a way to pull info from the container, and store it against the cache? If we notice the container hash/etc is different we can invalidate the cache, even with optimise=true

Which drush version do you use?
For me in 📌 Refactoring hux services breaks container rebuild Closed: cannot reproduce I did have problems with drush 12, but not with drush 11.
In cr in drush 12, it tries to make a full bootstrap before it would rebuild the container. In 11 it does not.

We should bring this up in the drush issue queue.

@donquixote we were hitting this issue on drush 11

On second thought, it makes sense why this issue is different than the one I opened.

Perhaps theres a way to pull info from the container, and store it against the cache? If we notice the container hash/etc is different we can invalidate the cache, even with optimise=true

I don't know if there is anything in the container that is always renewed.
But we could generate a random value or set a timestamp in the compiler pass, and use that to invalidate the cache.

We just started using this module on Omnipedia and it's working great so far - awesome work by the way - but I did run into this issue as well and only a complete cache clear seemed to work when I removed a hook class to move it to another module today. I don't like clearing caches in production, but it looks like just invalidating/rebuilding the container doesn't seem to work here so I'm interested in potential solutions and helping out if I can be of use.

AmbientImpact in the meantime you can add something like this to your deployment:

drush ev "\Drupal::service('cache.bootstrap')->delete('hux.discovery');"

@mstrelan Awesome. Thanks!

So I did a quick search in core to see how the deployment identifier works and it's actually really simple: it's used as part of the cache key for the container in DrupalKernel::getContainerCacheKey():

$parts = ['service_container', $this->environment, \Drupal::VERSION, Settings::get('deployment_identifier'), PHP_OS, serialize(Settings::get('container_yamls'))];
return implode(':', $parts);

It looks like @acbramley in #5 🐛 Removing a Hux hook implementation/class can cause fatal errors Needs review had the right idea. The main problem is that I don't know if there's a cache tag we can add to the Hux discovery cache item that gets invalidated when the container is rebuilt - though if there is one, we could use that - so the fallback option could be to use the deployment identifier as part of the Hux discovery cache key like core uses for the container, which would automatically result in a cache miss when the deployment identifier changes. The downside is that the cache key now needs to be fetched from a method similar to core's, but that feels like a minor issue. What do you all think?

I had a chance to implement a similar method to core's in the issue fork and at least in my local testing it seems to work as intended, but I haven't had a chance to throw together a test for it yet but will get back to this tomorrow to do that. The core test for reference is \Drupal\Tests\system\Functional\DrupalKernel\ContainerRebuildWebTest

We just started using this module on Omnipedia and it's working great so far - awesome work by the way -

Thank you for the kind words.

-

I'm still hoping we'd be able to add discovery data to the container itself, perhaps replacing the need for Hux to utilise cache bins directly, as suggested by #8. That way if the container is invalidated, Hux data goes along with it.

That would necessitate moving the \Drupal\hux\HuxModuleHandler::discovery out of a method call and doing it in \Drupal\hux\HuxCompilerPass::process, or something along those lines.

@dpi Fair enough. Will watch the issue and help out if I'm able if/when you decide how to proceed.

Following up from #3401023-8: Implement HuxModuleHandler::resetImplementations() as a public way to trigger discovery of hook classes? → : I'm only familiar with the basics of how the Drupal/Symfony container works at a low level, so I'm wondering if anyone can point me in the right direction for how to store Hux data in the container as mentioned. Been poking through the Drupal core code and the Symfony container documentation, but not sure where to start.

@Ambient.Impact You're at the same point I'm at ;)

No worries. I'll dig into this when I have some time/energy to spare. Thanks!

hey @Ambient.Impact et al, can you see if MR!23 solves your issues?

Confirmed by applying the patch, warming caches, changing a method name and clearing cache. Passed as expected. Following the same steps without the patch throws the Uncaught TypeError: Failed to create closure from callable error

Thanks @acbramley for checking this out.

Merged.

I'm going to put this in a new 1.3 branch w/ beta stability just in case something goes awry.

In the meantime I'd appreciate further feedback if it doesnt work for you.

Totally missed this. Awesome to see it make it into main. I was actually looking at core yesterday and wondering how to better sync this up with core rebuilding the container, so I was looking at Drupal\Core\DrupalKernel::invalidateContainer() and saw it just outright deletes the whole container cache bin:

$this->bootstrapContainer->get('cache.container')->deleteAll();

I know the discovery cache bin probably makes more sense in a lot of ways, but what if we cached to the container cache bin? Would anything in core break and/or is there anything in core telling us it would be a bad idea? Given how low level Hux aims to be, I think there's an argument that we can push that envelope a little since (one can dream) this can be integrated into core one day.

I’m pretty happy with how this turned out, this solution works out 🤞 I’d rather not deal with container x cache bin/factory services outside of core.

No worries! I just had a look at the code and it looks like you figured out how to add the discovery stuff to the container itself?

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