8000 Modernize `strpos()` calls by derrabus · Pull Request #9480 · doctrine/orm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Modernize strpos() calls #9480

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
Merged
merged 1 commit into from
Feb 7, 2022
Merged

Modernize strpos() calls #9480

merged 1 commit into from
Feb 7, 2022

Conversation

derrabus
Copy link
Member
@derrabus derrabus commented Feb 5, 2022

PHP 8 introduced nicer function to check if a string contains another string. The PHP 8 polyfill that we're already using should allow us to leverage those function.

@derrabus derrabus added this to the 2.12.0 milestone Feb 5, 2022
greg0ire
greg0ire previously approved these changes Feb 6, 2022
View reviewed changes
Copy link
Member
@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

The amount of changes you did shows that it was totally worth creating these new functions!

@greg0ire greg0ire closed this Feb 6, 2022
@greg0ire greg0ire reopened this Feb 6, 2022
@greg0ire
Copy link
Member
greg0ire commented Feb 6, 2022 •
edited
Loading

Triggered a new build, this one wasn't starting

It seems to be failing because of a signature-compatibility issue, not sure why this was not caught before in #9418 🤔

Declaration of Doctrine\Tests\ORM\Functional\ManyToOneOrphanRemovalTest::getEntityManager(?Doctrine\DBAL\Connection $connection = NULL, ?Doctrine\Persistence\Mapping\Driver\MappingDriver $mappingDriver = NULL): Doctrine\ORM\EntityManagerInterface should be compatible with Doctrine\Tests\OrmFunctionalTestCase::getEntityManager(?Doctrine\Tests\DbalExtensions\Connection $connection = NULL, ?Doctrine\Persistence\Mapping\Driver\MappingDriver $mappingDriver = NULL): Doctrine\ORM\EntityManagerInterface in /home/runner/work/orm/orm/tests/Doctrine/Tests/ORM/Functional/ManyToOneOrphanRemovalTest.php on line 95

- Doctrine\Tests\ORM\Functional\ManyToOneOrphanRemovalTest::getEntityManager(
+ Doctrine\Tests\OrmFunctionalTestCase::getEntityManager(
- ?Doctrine\DBAL\Connection $connection = NULL, 
+ ?Doctrine\Tests\DbalExtensions\Connection $connection = NULL
 ?Doctrine\Persistence\Mapping\Driver\MappingDriver $mappingDriver = NULL
 ): Doctrine\ORM\EntityManagerInterface

EDIT: it was in fact occuring, but is just a warning at that point: https://github.com/doctrine/orm/runs/4914222176?check_suite_focus=true

Maybe we should use https://phpunit.readthedocs.io/en/9.5/configuration.html?highlight=warning#the-convertwarningstoexceptions-attribute or https://phpunit.readthedocs.io/en/9.5/configuration.html?highlight=warning#the-failonwarning-attribute ?

@SenseException
Copy link
Member

Maybe we should use https://phpunit.readthedocs.io/en/9.5/configuration.html?highlight=warning#the-convertwarningstoexceptions-attribute or https://phpunit.readthedocs.io/en/9.5/configuration.html?highlight=warning#the-failonwarning-attribute ?

In this PR or in another one?

@derrabus derrabus force-pushed the improvement/modernize_strpos branch from c9ea3dd to 6d7dd3a Compare February 6, 2022 14:26
@greg0ire
Copy link
Member
greg0ire commented Feb 6, 2022

Another one would be best I suppose

@derrabus
Copy link
Member Author
derrabus commented Feb 6, 2022

The warning did not cause the tests to fail.

PHP Fatal error:  Uncaught Error: Call to a member function executeStatement() on null in /home/runner/work/orm/orm/tests/Doctrine/Tests/ORM/Functional/ValueConversionType/ManyToManyCompositeIdForeignKeyTest.php:53
Stack trace:
#0 /home/runner/work/orm/orm/vendor/phpunit/phpunit/src/Framework/TestSuite.php(751): Doctrine\Tests\ORM\Functional\ValueConversionType\ManyToManyCompositeIdForeignKeyTest::tearDownAfterClass()
#1 /home/runner/work/orm/orm/vendor/phpunit/phpunit/src/Framework/TestSuite.php(746): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#2 /home/runner/work/orm/orm/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(629): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#3 /home/runner/work/orm/orm/vendor/phpunit/phpunit/src/TextUI/Command.php(206): PHPUnit\TextUI\TestRunner->doRun(Object(PHPUnit\Framework\TestSuite), Array, true)
#4 /home/runner/work/orm/orm/vendor/phpunit/phpunit/src/TextUI/Command.php(162): PHPUnit\TextUI\Command->run(Array, true)
#5 phpvfscomposer:///h in /home/runner/work/orm/orm/tests/Doctrine/Tests/ORM/Functional/ValueConversionType/ManyToManyCompositeIdForeignKeyTest.php on line 53

I have no idea why this suddenly happens and why only on PHP 7.1.

@greg0ire
Copy link
Member
greg0ire commented Feb 6, 2022 •
edited
Loading

@derrabus Looking at the interrupted output of the non-prefer-lowest build, it seems it works better there. So it's probably more related to the prefer-lowest part than to the PHP 7.1 part. I'm not reproducing the issue locally with prefer-lowest, so the PHP 7.1 part does play a role. Maybe it has to do with the version of symfony/polyfill-php80?

@greg0ire
Copy link
Member
greg0ire commented Feb 6, 2022

str_start_with is only available since v1.16

@greg0ire greg0ire force-pushed the improvement/modernize_strpos branch from 6d7dd3a to 7499cb2 Compare February 6, 2022 22:22
@derrabus
Copy link
Member Author
derrabus commented Feb 7, 2022

Oh, right. I should've checked that. Thanks!

@derrabus derrabus merged commit 978f687 into doctrine:2.12.x Feb 7, 2022
@derrabus derrabus deleted the improvement/modernize_strpos branch February 7, 2022 08:34
derrabus added a commit to derrabus/orm that referenced this pull request Feb 7, 2022
@derrabus
Merge branch '2.12.x' into 3.0.x
607911f 
* 2.12.x:
  Modernize strpos() calls (doctrine#9480)
  Fix types on persisters (doctrine#9466)
  Rename DoctrineSetup to ORMSetup (doctrine#9481)
  Remove useless catches
n-e-m-a-nj-a pushed a commit to n-e-m-a-nj-a/orm that referenced this pull request Mar 29, 2022
@derrabus @n-e-m-a-nj-a
Modernize strpos() calls (doctrine#9480)
a36bb4a
@github-actions github-actions bot mentioned this pull request Apr 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire approved these changes

Assignees
No one assigned
Labels
Improvement
Project 45F7 s
None yet
Milestone
2.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@derrabus @greg0ire @SenseException
0