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

Fix path on windows #63

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Fix path on windows #63

merged 3 commits into from
Sep 27, 2023

Conversation

raziel057
Copy link
Contributor

While running my project test suite on Windows (local env) and linux (CI env) I could notice that I have a deprecation raised on Linux that is not on Windows.

Indirect deprecation triggered by MyControllerTest::testDelete:
#27 53.01 AbstractPlatform::supportsForeignKeyConstraints() is deprecated. (AbstractPlatform.php:4093 called by JoinedSubclassPersister.php:254,
https://github.com/doctrine/dbal/pull/5409, package doctrine/dbal)
#27 53.01 Stack trace:
#27 53.01 #0 [internal function]: Symfony\Bridge\PhpUnit\DeprecationErrorHandler->handleError(16384, '...', '...', 217)
#27 53.01 #1 vendor/doctrine/dbal/src/Platforms/AbstractPlatform.php(4093): Doctrine\Deprecations\Deprecation::triggerIfCalledFromOutside('...',
'...', '...')
#27 53.01 #2 vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/JoinedSubclassPersister.php(254): Doctrine\DBAL\Platforms\AbstractPlatform->s
upportsForeignKeyConstraints()
#27 53.01 #3 vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php(1296): Doctrine\ORM\Persisters\Entity\JoinedSubclassPersister->delete(Object(...))

When the path is created on Windows using the package name (ex.: doctrine/dbal) it results as \vendor\doctrine/dbal\ but the real path to the library coming from $backtrace[1]['function'] is ...\vendor\doctrine\dbal\src\Platforms\AbstractPlatform.php so currently it match the following condition on windows while it's not the case on Linux.

if (strpos($backtrace[0]['file'], $path) === false) {
    return;
}

After, that been say, I'm not sure if it's normal that I got the deprecation as it's raised from a call to the deprecated method supportsForeignKeyConstraints() of AbstractPlatform of the doctrine/dbal package done from the delete method of vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/JoinedSubclassPersister.php.

@raziel057
Copy link
Contributor Author

After, that been say, I'm not sure if it's normal that I got the deprecation as it's raised from a call to the deprecated method supportsForeignKeyConstraints() of AbstractPlatform of the doctrine/dbal package done from the delete method of vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/JoinedSubclassPersister.php.

Well in fact it's normal as the method is called from doctrine/orm. We need to wait doctrine ORM 3.x to avoid the deprecation.

@derrabus
Copy link
Member

derrabus commented Sep 6, 2023

I'm curious if we need to write a test for this issue. Would it be enough to run the current test suite on a Windows runner in order to detect this kind of problem?

@raziel057
Copy link
Contributor Author

raziel057 commented Sep 8, 2023

@derrabus I checked running the testDeprecationIfCalledFromOutside unit test on Windows and yes we can detect the issue. Currently the test is marked as risky because the expectErrorHandler doesn't calls fail in case no error is raised.

On windows without the fix:

vendor/bin/phpunit tests/Doctrine/Deprecations/DeprecationTest.php --filter=testDeprecationIfCalledFromOutside
PHPUnit 9.6.11 by Sebastian Bergmann and contributors.

R..                                                                 3 / 3 (100%)

Time: 00:00.025, Memory: 6.00 MB

There was 1 risky test:

1) Doctrine\Deprecations\DeprecationTest::testDeprecationIfCalledFromOutside
This test did not perform any assertions

X:\workspace-novento\deprecations\tests\Doctrine\Deprecations\DeprecationTest.php:229

OK, but incomplete, skipped, or risky tests!
Tests: 3, Assertions: 2, Risky: 1.

On windows with the fix:

vendor/bin/phpunit tests/Doctrine/Deprecations/DeprecationTest.php --filter=testDeprecationIfCalledFromOutside
PHPUnit 9.6.11 by Sebastian Bergmann and contributors.

...                                                                 3 / 3 (100%)

Time: 00:00.022, Memory: 6.00 MB

OK (3 tests, 4 assertions)

I'm going to update the testDeprecationIfCalledFromOutside test to make it fail explicitly if the getUniqueTriggeredDeprecationsCount is not equals to 1.

@raziel057
Copy link
Contributor Author

With the new assert on Windows, without the fix of this PR, we have the following result:

vendor/bin/phpunit tests/Doctrine/Deprecations/DeprecationTest.php --filter=testDeprecationIfCalledFromOutside
PHPUnit 9.6.11 by Sebastian Bergmann and contributors.

F..                                                                 3 / 3 (100%)

Time: 00:00.021, Memory: 6.00 MB

There was 1 failure:

1) Doctrine\Deprecations\DeprecationTest::testDeprecationIfCalledFromOutside
Failed asserting that 0 matches expected 1.

X:\workspace-novento\deprecations\tests\Doctrine\Deprecations\DeprecationTest.php:238

@raziel057
Copy link
Contributor Author

Hello, I just noticed the same kind of issue with another project I currently upgrade. I have the following deprecation raised on Linux (in CI server) that is not raised when running my tests locally on Windows.

4x: The Doctrine\ORM\Event\LifecycleEventArgs class is deprecated and will be removed in ORM 3.0. Use Doctrine\Persistence\Event\LifecycleEventArgs instead. (LifecycleEventArgs.php:24 called by ORM.php:192, https://github.com/doctrine/orm/issues/9875, package doctrine/orm)
    2x in ClientControllerTest::testDelete from App\Tests\Controller
    2x in ClientUserControllerTest::testDelete from App\Tests\Controller

If I apply this patch, I get the same deprecation on Linux and Windows. Any chance to merge the fix?

@malarzm malarzm added this to the v1.1.2 milestone Sep 27, 2023
@malarzm malarzm added the bug Something isn't working label Sep 27, 2023
@malarzm malarzm merged commit 4f2d4f2 into doctrine:1.1.x Sep 27, 2023
12 checks passed
@malarzm
Copy link
Member

malarzm commented Sep 27, 2023

Sure, thanks @raziel057!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants