Skip to content

Deprecated ChainConfigResolver::addResolver() method#763

Open
Steveb-p wants to merge 4 commits into
5.0from
chain-config-resolver-deprecations
Open

Deprecated ChainConfigResolver::addResolver() method#763
Steveb-p wants to merge 4 commits into
5.0from
chain-config-resolver-deprecations

Conversation

@Steveb-p

Copy link
Copy Markdown
Contributor
🎫 Issue IBX-XXXXX

Description:

ChainConfigResolver should not change at runtime. Resolvers should only be added during construction, and should not be changed after.

Otherwise, the ChainConfigResolver becomes stateful and can potentially cause issues in contexts where application should reset it's state, like in Messenger workers.

Warning

When merging up, remove deprecations.

For QA:

Documentation:

@sonarqubecloud

Copy link
Copy Markdown

* @dataProvider addResolverProvider
*/
public function testAddResolver($declaredPriority, $expectedPriority)
public function testTaggedResolverAddedToConstructor(?int $declaredPriority, int $expectedPriority): void

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could a little expand that test to test also different $declaredPriority per each definition - we expect to put them into separate array groups 😉

I think we could test both scenarios (the same declaredPriority and different ones) with using single dataProvider - but splitting these priorities per each definition

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I intend to remove this entirely in 6.x, so I didn't want to put more tests in place (only what I've noticed I've missed in implementation when I looked at the test code yet again).

I want to replace it with tagged_iterator with priority, and make ChainConfigResolver completely not aware that priority exists (as implementation), so those tests would become irrelevant (and CompilerPass too).

@alongosz alongosz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, Yes, Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants