Problem/Motivation

JSON API fails against 8.5.x, because JSON API has a class implementing RouteEnhancerInterface, which is deprecated since #2883680: Force all route filters and route enhancers to be non-lazy.

Using deprecated APIs causes test failures since #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code.

But the interface we should implement instead is only available in 8.5 and newer. So there's not really a way for the JSON API to have green tests against 8.5.

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#13 2926413-13.patch9.72 KBwim leers
#13 interdiff.txt31.5 KBwim leers
#2 2926413-2.patch35.3 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2926413-2.patch35.3 KB

The only solution for now is to explicitly express the expected deprecations for every test method, and marking the test as being @group legacy.

JSON API against 8.5 core has 17 fails. This should have zero.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2926409: BaseFieldDefinition::setQueryable() is deprecated in Drupal 8.4.0, +#2926415: Request::isMethodSafe() is deprecated in Symfony 3.2/Drupal 8.4.0

#2926409: BaseFieldDefinition::setQueryable() is deprecated in Drupal 8.4.0 and #2926415: Request::isMethodSafe() is deprecated in Symfony 3.2/Drupal 8.4.0 were deprecation fixes that were actually fixable, but were made much harder to analyze due to this one.

Status: Needs review » Needs work

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

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Priority: Normal » Major

But the interface we should implement instead is only available in 8.5 and newer. So there's not really a way for the JSON API to have green tests against 8.5.

Wow! That's very inconvenient.

Is there an issue to track these issues in Drupal core? I have already seen people moving away from Drupal because things break between minor releases. Maybe I can help there.

Bumping priority since having green tests is very important to the project.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Is the patch in #2 supposed to come back green? From your comment I think so, but the testbot returned red.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Related issues: +#2922537: "User deprecated function" should not cause a test to fail
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues:

Is there an issue to track these issues in Drupal core?

I linked to it in the IS: #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code and its related issues, and specifically #2607260: [meta] Core deprecation message introduction, contrib testing etc..

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2607260: [meta] Core deprecation message introduction, contrib testing etc.

c/p fail

Wim--

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Bumping priority since having green tests is very important to the project.

Totally agree! Which is why I've been working to get it back to green all morning.

Is the patch in #2 supposed to come back green? From your comment I think so, but the testbot returned red.

It comes back green on 8.5 (which is where the deprecation failures were occurring), but it comes back red on 8.4, because the expected deprecations don't actually occur there.

Which makes sense … but … it means it's currently impossible to have tests pass on both 8.4 and 8.5 :( :(

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Adding @group legacy will turn off the fails due to deprecation errors.

Adding @expectedDeprecation will fail the test if the deprecation error does not happen.

You only need them both if you are testing whether the deprecation error occurs.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Oh… but … testing locally, adding @group legacy still causes failures. I only get no failures if I also add @expectedDeprecation.

I guess I'm running tests wrong somehow?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt31.5 KB
new 2926413-13.patch9.72 KB

Let's try #11 then.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Okay, that works. In the interest of getting JSON API's tests to green again, committing this right away. But this still fails for me locally :(

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Crediting @Mile23.

  • Wim Leers committed 9599769 on 8.x-1.x 
    Issue #2926413 by Wim Leers, Mile23: RouteEnhancerInterface is...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

After that commit, I see this:

$ ./vendor/bin/phpunit -c core/ --group jsonapi --stop-on-fail
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing 
...............................................................  63 / 307 ( 20%)
............................................................... 126 / 307 ( 41%)
............................................................... 189 / 307 ( 61%)
............................................................... 252 / 307 ( 82%)
.......................................................

Time: 8.62 minutes, Memory: 236.00MB

OK (307 tests, 1007 assertions)

Legacy deprecation notices (136)

I guess you should composer install for 8.5.x.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Alex Pott pointed out that PHPStorm's "run tests" functionality overrides certain PHPUnit things, and that's probably why it didn't work for me.

Status: Fixed » Closed (fixed)

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