Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add result cache meta extension for DI container #421

Open
wants to merge 5 commits into
base: 2.0.x
Choose a base branch
from

Conversation

Wirone
Copy link
Contributor

@Wirone Wirone commented Jan 7, 2025

Important

This feature was brought to you thanks to GetResponse - Marketing Software for Professional Email Marketing, which paid for my work on this.

Fixes #255, continues phpstan/phpstan-src#3765.


Following this experiment's results I decided to calculate meta hash based on DI container's actual content (params and services), without interacting with XML file directly. ParameterMapFactory and ServiceMapFactory were used for retrieving data, also I implemented "singleton" approach there, so the XML file won't be processed multiple times (once for calculating hash, then for processing rules).

I start with a draft PR so I can get early feedback. I also need help when it comes to testing this, because I don't see e2e approach here. I wanted to test it on our codebase, but it's really cumbersome to setup Composer dependencies to get dev version of PHPStan and the plugin 😅.

@Wirone
Copy link
Contributor Author

Wirone commented Jan 8, 2025

@ondrejmirtes did you have opportunity to look at it? I hope for your feedback as in the main extension point PR, so I can finish this properly 🙂.

@ondrejmirtes
Copy link
Member

It's a bit crazy to urge me less than 24h after you opened the PR. Please avoid that next time.

A literally wasn't in front of my computer since then.

@Wirone
Copy link
Contributor Author

Wirone commented Jan 8, 2025

Sorry, I just didn't know you were notified about the PR because I realised I did not mention you in the description 😅. Sorry for bothering, take your time of course, just wanted to know if you're aware about this one.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can test this by:

  1. Create SymfonyContainerResultCacheMetaExtension extends PHPStanTestCase
  2. Create a new .neon file under tests/ and link to an actual dumped container XML file which you're going to commit in this repository under tests/.
  3. Override getAdditionalConfigFiles method and supply path to the .neon file from 2)
  4. Create new SymfonyContainerResultCacheMetaExtension and supply constructor arguments via self::getContainer()->getByType()

src/Symfony/XmlServiceMapFactory.php Outdated Show resolved Hide resolved
Wirone added 2 commits January 8, 2025 19:15
Otherwise it may fail with `foreach() argument must be of type array|object, null given`.
@Wirone Wirone force-pushed the codito/result-cache-meta-extension branch from ea69633 to 2bc5a26 Compare January 8, 2025 18:42
@Wirone
Copy link
Contributor Author

Wirone commented Jan 8, 2025

@ondrejmirtes please make sure you're familiar with the commits' messages, as I provided additional info there. When it comes to serialize() I believe it's more stable than var_export() when it comes to producing string that is used for calculating hash. We could use json_encode() too, but it would require adding ext-json to Composer's dependencies.

Again, thank you very much for providing feedback and guiding about tests 🍻.

@Wirone Wirone marked this pull request as ready for review January 8, 2025 18:47
@Wirone Wirone requested a review from ondrejmirtes January 8, 2025 18:47
This test ensures that hash calculated for Symfony's DI container remains the same or changes under provided conditions. This test is significantly slower than other unit tests, this is caused by rebuilding Nette container for each Symfony DI container's XML content - it's required in order to get fresh parameter/service maps from `self::getContainer()->getByType()`, because `self::getContainer()` caches containers for each `self::getAdditionalConfigFiles()` unique set, and with the same Nette config/container it would be retrieved from cache, so the hash correctness couldn't be verified properly.
@Wirone Wirone force-pushed the codito/result-cache-meta-extension branch from 2bc5a26 to 9393cf7 Compare January 8, 2025 19:13
@Wirone Wirone force-pushed the codito/result-cache-meta-extension branch from 9393cf7 to 5f8002d Compare January 8, 2025 19:20
@Wirone
Copy link
Contributor Author

Wirone commented Jan 8, 2025

To be honest, I am not sure whether services' tags should be taken into consideration when calculating container's hash 🤔. Symfony\Component\DependencyInjection\ContainerInterface and even Symfony\Component\DependencyInjection\Container doesn't expose any API for interacting with tags, these are rather used in compiler passes during container build. But since PHPStan\Symfony\ServiceDefinition contains tags, I included them in the calculation. Let me know what you think @ondrejmirtes 🙂.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Result cache is not invalidated after DI container dump
2 participants