Proposal: Add config option to disable DI definition change cache invalidation `invalidateResultCacheOnDiDefinitionChange`

[santysisi]

Hi 👋 First of all, thank you for the great work on PHPStan and the Symfony extension!

We are working on integrating result cache in our project to speed up analysis times. However, we noticed that the cache is invalidated very frequently, which reduces its effectiveness in most of our development workflows.

After some investigation, we identified the source of this behavior to be the feature added in this PR

We completely understand the value of this feature, it ensures the cache is invalidated when DI definitions change, which is especially important for production builds or CI pipelines to guarantee correctness.

However, in our development setup, this behavior is too aggressive. The DI container in our Symfony app changes very frequently , and as a result, the cache is invalidated most of the time. This defeats the purpose of caching during active development.

We suggest introducing a new configuration option (which was once discussed in the original PR but never merged):

[ruudk]

Thank you for raising this. I've had so many situations where I thought "why does PHPStan suddenly do a full analysis from scratch?" and then remembering this feature being the cause for it. Sometimes, I make changes to services that are covered by Symfony's Autowire and that triggers a rebuild. For example, I inject a new dependency into a class, or I add an attribute.

Most of the time, this does not affect PHPStan at all. I would say, rarely it does. In our setup, we have to enable the Symfony Container integration for dead-code-detector to work. Besides that, we rarely fetch services on a container, only in a few tests. If it wasn't for dead-code-detector I would have already turned off the Container support in order to prevent this cache invalidation from happening.

So I'm 👍 for an option to disable this! 😁

[ondrejmirtes]

The solution isn't to disable this key altogether, but instead take a look at the dumped container and do a smarter hash of the contents - e.g. change the hash only when there's a chance it can have impact on PHPStan's results.

[santysisi]

Hi, thanks for your response!

First of all, I want to mention that I don't have deep knowledge of PHPStan, so I apologize if anything I say is incorrect.

While looking through the project, I noticed a couple of things that might be worth considering (though I might be wrong):

1. Removing the 'alias' key from $services in SymfonyContainerResultCacheMetaExtension
   I observed that the getAlias method is only used in the XmlServiceMapFactory class, and it's primarily used to create new Service and add this new Service to the $services. It seems like the 'alias' key might not be necessary anymore, since a new $service will be created if this change is relevant.
2. Reducing the size of $serviceTags in SymfonyContainerResultCacheMetaExtension
   I observed that the getTags method is only used in the In the MessageMapFactory class, and it looks like the getTags method only considers tags with the name 'messenger.message_handler' and the attribute 'handles'. Based on this, we might be able to reduce the size of $serviceTags by filtering out only the tags with that specific name and attribute

Again, I don't have a deep understanding of the project, but if you agree with these changes, I'd be happy to work on a PR.

Thanks again!

[ruudk]

The solution isn't to disable this key altogether, but instead take a look at the dumped container and do a smarter hash of the contents - e.g. change the hash only when there's a chance it can have impact on PHPStan's results.

The cache extension is fine. The container did change. For example, a service definition changed, because it takes a new dependency.

The issue is, often it's unrelated for the working of this phpstan-symfony extension. In our project, it helps with detecting ±6 Container::get calls, and that's it. That means that when making a change to a service, PHPStan has to do a full analysis again, only to come to the same conclusion: no errors. I hear so many complaints from my colleagues because of this issue. A single line busts the whole thing.

So that's why I removed the containerXmlPath configuration for phpstan-symfony to tackle it. But it requires a change in dead-code-detector to decouple it properly:

• Symfony: allow explicit containerXmlPaths configuration shipmonk-rnd/dead-code-detector#325

[janedbal]

I did some analysis in our CI (about 1000 recent jobs) and turns our we utilize PHPStan cache only in 24% cases (mainly) due to this problem.

[ondrejmirtes]

Would be nice to track down which changes matter and which don’t so that we don’t invalidate on every change.

Maybe we can always unset some ever-changing parameters. But I suspect we should actually track down (using collectors?) what the analysed code asks of the Symfony DIC to know what should be invalidated and what should not.

[ondrejmirtes]

My idea was: understand that the analysed code only ever asks about service X and parameter Y. Invalidate the result cache through the meta extension only when these two things change. The whole project would get reanalysed, but only when these things change.

The submitted PR #478 has a different approach. It wants to track which files ask the DI container about anything, and when the DI container changes (with any minor change), it makes these files (and only them) reanalyse.

This has to be carefully considered. I'm worried that the current behaviour is good because it reanalyses the whole project and the result cache could become stale for some reason if we don't reanalyse it in full.

At the same time, I feel like the two approaches might get combined for the best outcome - track which services and parameters are accessed in a file, and reanalyse that file only when those specific service and parameters change. But that's very aggressive and also maybe prone to bugs.

Also note that ParameterMap and ServiceMap are @api and might be used for some non-obvious outcomes, so reanalysing the whole project might still be for the best. I've been using this example in my talks about custom rules for years. "When doing new Person, check that Person is not a DI-registered service. If it is, report an error and tell the user to inject it into the constructor instead.". So suddenly, new Person means that we're esentially checking $container->get(Person::class), but this fact would be lost with the proposed changes in this issue.

[janedbal]

I think the core logic should try to be as precise as possible - only invalidate as little as possible. If somebody for some reason needs the old bad behaviour, some ResultCacheMetaExtension can still be used in userland for old simple behaviour.

I just think ResultCacheMetaExtension do not offer what real applications need and there should be a better way to track real external dependencies. The #478 shows one of the possible ways to do it.