Add result cache meta extension for DI container#421
Add result cache meta extension for DI container#421ondrejmirtes merged 7 commits intophpstan:2.0.xfrom
Conversation
|
@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 🙂. |
|
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. |
|
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. |
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
You can test this by:
- Create
SymfonyContainerResultCacheMetaExtension extends PHPStanTestCase - 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/.
- Override
getAdditionalConfigFilesmethod and supply path to the .neon file from 2) - Create
new SymfonyContainerResultCacheMetaExtensionand supply constructor arguments viaself::getContainer()->getByType()
Wirone
force-pushed
the
codito/result-cache-meta-extension
branch
from
ea69633 to
2bc5a26
Compare
|
@ondrejmirtes please make sure you're familiar with the commits' messages, as I provided additional info there. When it comes to Again, thank you very much for providing feedback and guiding about tests 🍻. |
Wirone
force-pushed
the
codito/result-cache-meta-extension
branch
2 times, most recently
from
9393cf7 to
5f8002d
Compare
|
To be honest, I am not sure whether services' tags should be taken into consideration when calculating container's hash 🤔. |
Wirone
force-pushed
the
codito/result-cache-meta-extension
branch
from
81751fb to
b6bb9a9
Compare
|
I will also get rid of the Configuration class in 2.0.x now. |
Otherwise it may fail with `foreach() argument must be of type array|object, null given`.
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
force-pushed
the
codito/result-cache-meta-extension
branch
from
b6bb9a9 to
192e11e
Compare
| $services = []; | ||
| /** @var Service[] $aliases */ | ||
| $aliases = []; | ||
| foreach ($xml->services->service as $def) { |
There was a problem hiding this comment.
What are these changes for? Can you remove them?
Wirone
force-pushed
the
codito/result-cache-meta-extension
branch
from
8f5d9f1 to
ad83070
Compare
|
Thank you. |
2 participants
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.
ParameterMapFactoryandServiceMapFactorywere 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 😅.