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

Resolve controller resolver auto_mapping test deprecations #1778

Closed

Conversation

bobvandevijver
Copy link
Contributor

Fixes the introduced test deprecations caused by the changed default of the controller resolver auto mapping value. Lines that have been changed to reflect the new default value have been annotated with a todo so they can be remove when preparing the 3.0 release.

As requested by @dmaicher in #1762 (comment).

Fixes the introduced test deprecations caused by the changed default
of the controller resolver auto mapping value. Lines that have been
changed to reflect the new default value have been annotated with a todo
so they can be remove when preparing the 3.0 release.
@bobvandevijver bobvandevijver force-pushed the fix-test-deprecations branch from 686466c to 2fb31bb Compare March 19, 2024 08:29
@bobvandevijver bobvandevijver marked this pull request as draft March 19, 2024 08:35
@bobvandevijver
Copy link
Contributor Author

bobvandevijver commented Mar 19, 2024

@dmaicher I am not sure if this is the right approach. I thought there were only deprecations in the DoctrineExtensionTest, but they are all over the place, also in tests that do not touch the configuration at all (for example the ContainerTest).

All these deprecation messages disappear automatically when the default value is being changed in 3.0. Would it be acceptable to keep the deprecation notices, and only keep the single real test change (https://github.com/doctrine/DoctrineBundle/pull/1778/files#diff-95cb7e648b59313419d71246ec5fa0a5c4b6c917bd286b3c71f13d826c9e5b2aR635) so that the tests stay green after the default has changed?

@dmaicher
Copy link
Contributor

Or if its too much of a hassle to fix all of them maybe we can also add a baseline and ignore them? @ostrolucky wdyt?

@bobvandevijver
Copy link
Contributor Author

That would require a different baseline for every different test run, which is not something I would prefer. Maybe a better option would be to update the DoctrineExtension to detect whether PHP unit is running, and then apply the new default already for the tests?

Something like:

                if (defined('PHPUNIT_DOCTRINE_BUNDLE_TESTSUITE')) {
                    $config['controller_resolver']['auto_mapping'] = false;
                } else {
                    trigger_deprecation('doctrine/doctrine-bundle', '2.12', 'The default value of "doctrine.orm.controller_resolver.auto_mapping" will be changed from `true` to `false`. Explicitly configure `true` to keep existing behaviour.');
                    $config['controller_resolver']['auto_mapping'] = true;
                }

When combined with the following in the phpunit configuration, that would apply the new default already during tests:

    <php>
        <const name="PHPUNIT_DOCTRINE_BUNDLE_TESTSUITE" value="true"/>
    </php>

@ostrolucky
Copy link
Member

Thanks for the help. But indeed, looks like we would have to change more places than I'm comfortable. So I've taken the liberty to go with a baseline in the end. It shouldn't require us to have different baseline every different test run either.

@bobvandevijver
Copy link
Contributor Author

Alright, works for me! I will submit another PR with just the change for the test where setting auto mapping to true would actually be relevant for the test to succeed.

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.

3 participants