Skip to content

[Dependency Injection] XmlFileLoader treats when@env differently than YamlFileLoader #58293

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.

Sign up for GitHub

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

Jump to bottom
Closed
CipherdevNL opened this issue Sep 17, 2024 · 3 comments
Closed

[Dependency Injection] XmlFileLoader treats when@env differently than YamlFileLoader #58293

CipherdevNL opened this issue Sep 17, 2024 · 3 comments
Labels
Bug DependencyInjection Status: Needs Review

Comments

@CipherdevNL
Copy link

Symfony version(s) affected

5.4.43

Description

Inside the development environment we want to replace specific services with logbook variant to prevent accidentally calling remote api's. For the service configuration we mainly use the XML format, in which we tried the following:

<container xmlns="http://symfony.com/schema/dic/services"
           xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
           xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
    <services>
        <service id="Vendor\RemoteApiCaller" alias="Vendor\LogbookRemoteApiCaller"/>

        <service id="Vendor\HttpRemoteApiCaller" class="Vendor\HttpRemoteApiCaller"/>
        <service id="Vendor\LogbookRemoteApiCaller" class="Vendor\LogbookRemoteApiCaller"/>
    </services>

    <when env="prod">
        <services>
            <service id="Vendor\RemoteApiCaller" alias="Vendor\HttpRemoteApiCaller"/>
        </services>
    </when>
</container>

But we discovered that the Dependency Injection still want's to use the HttpRemoteApiCaller for non-prod environments which isn't expected.

After some additional debugging we discovered that the XmlFileLoader::class doesn't ignore the <services /> elements that are part of the <when env="..." /> element. Resulting in applying all service definition in order, and thus overwriting the defined value in the base <services /> element.

To make sure this isn't expected behaviour i've rewrote the same service config in Yaml to see what happens inside the YamlFileLoader::class. And as expected this loader only load the services defined in the when@prod: statement when the application environment is set to it.

services:
  Vendor\RemoteApiCaller:
    alias: Vendor\LogbookRemoteApiCaller

  Vendor\LogbookRemoteApiCaller:
    class: Vendor\LogbookRemoteApiCaller

  Vendor\HttpRemoteApiCaller:
    class: Vendor\HttpRemoteApiCaller

when@prod:
  services:
    Vendor\RemoteApiCaller:
      alias: Vendor\HttpRemoteApiCaller

I'm not that familiar with xPath but it seems like the query used here https://github.com/symfony/dependency-injection/blob/7.1/Loader/XmlFileLoader.php#L137 isn't excluding elements that are inside a <when /> element. Which also happens when passing it along inside http://xpather.com/ .

How to reproduce

See https://github.com/CipherdevNL/di-symfony-xml-yaml with an example, run the bug.php script with php bug.php to see the difference in processing.

Possible Solution

Make the xpath check more stricter to ignore the when statements. Haven't tested it with the latest Symfony 7.1 but based on the code diff I don't see any changes in the XmlFileLoader that resolves this issue.

When fixing this we should handle it as a BC as this inconsistency is already present in SF 5.4. So projects can (unintentionally) depend on this behaviour for their XML configs.

Additional Context

Output of the bug.php:

[XML] Expects 'logbook' got 'http'
[YAML] Expects 'logbook' got 'logbook'
[ALL] Different file loaders don't have the same result D:
@CipherdevNL
Copy link
Author

Have checkout the example repository on another dev machine that runs php 8 and supports SF 7.1, and it gives the same result as on PHP7 & SF5.4.

@nicolas-grekas
Copy link
Member

I'd ignore the BC aspect of this issue, it's clearly a bug.
Up to give a PR a try to fix it?

@CipherdevNL
Copy link
Author

Yes, I can give it a try to resolve it inside a PR. Will do an attempt after office hours. And ignore the BC aspect of it.

@CipherdevNL CipherdevNL mentioned this issue Sep 17, 2024
Merged
nicolas-grekas added a commit that referenced this issue Sep 28, 2024
@nicolas-grekas
bug #58299 [DependencyInjection] Fix XmlFileLoader not respecting w…
fe74ff3 
…hen env for services (Bradley Zeggelaar)

This PR was merged into the 5.4 branch.

Discussion
----------

[DependencyInjection] Fix `XmlFileLoader` not respecting when env for services

| Q             | A
| ------------- | ---
| Branch?       | 5.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | Fix #58293 <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

This PR resolves a bug inside the `XmlFileLoader` that causes the loading of all services inside a `<when env="..." />` elements even when not in the defined environment.

My knowledge about xPath is very limited, but it seems like the parser interpets `.//` as `//` (anywhere) while most like `./` (relative) was meant, due to the passing of an `$root` element. Based on the added test and passing of existing test this change seems like sufficient to respect the `<when env="...." />`

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

Commits
-------

2ec78d9 [DependencyInjection] Fix `XmlFileLoader` not respecting when env for services
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nicolas-grekas @xabbuh @carsonbot @CipherdevNL