Problem/Motivation

All update path tests should be legacy tests because they are testing upgrading data that might trigger deprecated code paths. However the new BrowserTestBase variants based on \Drupal\FunctionalTests\Update\UpdatePathTestBase don't trigger deprecation warnings. We need to find out why.

Proposed resolution

Update PHPUnit Bridge once https://github.com/symfony/symfony/pull/25685 is in.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#9 2934336-9.patch2.35 KBalexpott
#7 2934336-7.patch4.78 KBalexpott
#7 3-7-interdiff.txt2.79 KBalexpott
#3 2934336-3.patch3.13 KBalexpott
#3 2-3-interdiff.txt716 bytesalexpott
#2 2934336-2.patch2.43 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2934336-2.patch2.43 KB

Here's a test that is green when it shouldn't be. A similar test \Drupal\FunctionalTests\Core\Test\PhpUnitBridgeTest shows that BrowserTestBase supports deprecation testing. So something is special about \Drupal\FunctionalTests\Update\UpdatePathTestBase().

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2-3-interdiff.txt716 bytes
new 2934336-3.patch3.13 KB

So this looks like symfony fun... because changing the SYMFONY_DEPRECATIONS_HELPER to strict makes update path tests test deprecations again...

\Symfony\Bridge\PhpUnit\DeprecationErrorHandler::register() is determining that the deprecation error came from vendor and not from core :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

The problem is the interaction between weak vendors and how Symfony's deprecation helper works for tests that run in a separate process.

Status: Needs review » Needs work

The last submitted patch, 3: 2934336-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes

Created a PR to fix this in Symfony - https://github.com/symfony/symfony/pull/25685

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 3-7-interdiff.txt2.79 KB
new 2934336-7.patch4.78 KB

New symfony with the PR in has been released. It's working so this should fail. Hopefully there is only 1 fail.

Status: Needs review » Needs work

The last submitted patch, 7: 2934336-7.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2934340: Remove action deprecations from DeprecationListenerTrait::getSkippedDeprecations
StatusFileSize
new 2934336-9.patch2.35 KB

So yay that works... I think we should just make this issue about updating symfony/phpunit-bridge since that is where the bug lies. And then once #2916740: Add generic entity actions we fix all the update tests to be legacy tests and expected all the deprecations in #2934340: Remove action deprecations from DeprecationListenerTrait::getSkippedDeprecations

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: \Drupal\FunctionalTests\Update\UpdatePathTestBase doesn't trigger deprecation warnings » Update symfony/phpunit-bridge to the latest released version
chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented

Shouldn't we add the failing test from #7 to the current patch plus a setExpectedException?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@chr.fritsch nope - because if you set the expected deprecation it would work in HEAD. This is because the deprecation would be caught by yet another error handler (\Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerTrait::handleError()). So it would prove nothing :( And we can't add a failing test.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thanks a lot to the symfony community having such a fast process to fix issues, seriously!

+++ b/composer.lock
@@ -4214,6 +4214,10 @@
+                "thanks": {
+                    "name": "phpunit/phpunit",
+                    "url": "https://github.com/sebastianbergmann/phpunit"
                 }

This is interesting ...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +@deprecated

@alexpott: wow — thank you so much for your continued diligence and upstream PRs to fix problems before they even become problems for the Drupal community!

+++ b/composer.lock
@@ -4214,6 +4214,10 @@
+                "thanks": {
+                    "name": "phpunit/phpunit",
+                    "url": "https://github.com/sebastianbergmann/phpunit"
                 }

:)

  • catch committed d1e91cb on 8.5.x 
    Issue #2934336 by alexpott: Update symfony/phpunit-bridge to the latest...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed d1e91cb and pushed to 8.5.x. Thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Fixed » Active

#2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent was green and RTBC for >1 week. It failed just now. It looks like this change probably caused it to start failing… any help would be greatly appreciated :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Fixed

#17 was just a missing fullstop in the skipped deprecation messages added by the patch. This patch correctly fixed deprecation error handling.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

… and @alexpott filed two PRs due to this:

  1. https://github.com/symfony/symfony/pull/25752
  2. https://github.com/symfony/symfony/pull/25757

🎉

Thank you so much, @alexpott!

EDIT: looks like both of those PRs will ship with Symfony 3.4!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.