The JSON API core patch at #2843147-51: Add JSON:API to core as a stable module is failing tests because of this deprecation error in many dozens of the tests:

Remaining deprecation notices (1)

  1x: \Drupal\Core\Routing\Enhancer\RouteEnhancerInterface is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Instead, you should use \Drupal\Core\Routing\EnhancerInterface. See https://www.drupal.org/node/2894934

See the corresponding CR at https://www.drupal.org/node/2894934.

The tricky thing is that we can't fix this in JSON API 8.x-1.x because \Drupal\Core\Routing\EnhancerInterface only exists in 8.5. Only JSON API 8.x-2.x will require >=8.5.0.

CommentFileSizeAuthor
#12 interdiff-10.txt1002 bytesgabesullice
#10 2957271-10.patch4.78 KBgabesullice
#5 interdiff-4-5.txt1.73 KBgabesullice
#5 2957271-5.patch6.08 KBgabesullice
#4 interdiff-3-4.txt4.05 KBgabesullice
#4 2957271-4.patch4.61 KBgabesullice
#3 2957271-3.patch1.56 KBwim leers
#3 interdiff.txt832 byteswim leers
#2 2957271-2.patch811 byteswim 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 2957271-2.patch811 bytes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt832 bytes
new 2957271-3.patch1.56 KB

I forgot one.

gabesullice’s picture

gabesullice
Primary language English
commented
Issue tags: +API-First Initiative
StatusFileSize
new 2957271-4.patch4.61 KB
new interdiff-3-4.txt4.05 KB

This will break other routes that use the page parameter because applies will never be called again. See: #2961773-7: Update ParamEnhancers to not use deprecated interface to prevent unintentional interactions with requests

The attached fixes that.

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new 2957271-5.patch6.08 KB
new interdiff-4-5.txt1.73 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
  1. +++ b/src/Routing/Routes.php
@@ -189,6 +189,20 @@ class Routes implements ContainerInjectionInterface {
+  public static function isJsonApiRequest(array $defaults) {
+    return isset($defaults[RouteObjectInterface::CONTROLLER_NAME])
+      && $defaults[RouteObjectInterface::CONTROLLER_NAME] === static::FRONT_CONTROLLER;

    Checking

    '_is_jsonapi' => TRUE,

    would be simpler/nicer? It'd also allow custom modules to expand JSON API without being forced to use the same front controller. (Not recommended, but some custom projects may still want to do this.)

  2. +++ b/src/Routing/Routes.php
@@ -189,6 +189,20 @@ class Routes implements ContainerInjectionInterface {
+  public static function isJsonApiRequest(array $defaults) {

    Why public?

gabesullice’s picture

gabesullice
Primary language English
commented

It'd also allow custom modules to expand JSON API without being forced to use the same front controller. (Not recommended, but some custom projects may still want to do this.)

I think the original intent for this way of checking was to prevent expanding JSON API in unsupported ways.

Why public?

So that it can be used from both route enhancers. I suppose a trait could work, but that seemed like overkill. What do you think?

gabesullice’s picture

gabesullice
Primary language English
commented 
+++ b/src/Routing/JsonApiParamEnhancer.php
@@ -48,18 +47,14 @@ class JsonApiParamEnhancer implements RouteEnhancerInterface {
-    return $route->getDefault(RouteObjectInterface::CONTROLLER_NAME) == Routes::FRONT_CONTROLLER;

This is the code that the static method replaced.

wim leers’s picture

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

You're right, #6.1 is misguided because it's just moving existing logic into a separate method.

#6.2 makes sense too, I misread it first, my bad!

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new 2957271-10.patch4.78 KB

Needed a reroll, committing if tests pass.

  • gabesullice committed 7d07d79 on 8.x-2.x 
    Issue #2957271 by gabesullice, Wim Leers: [>=8.5] Fix...
gabesullice’s picture

gabesullice
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new interdiff-10.txt1002 bytes

Fixed the two CS violations.

Status: Fixed » Closed (fixed)

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

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Issue tags: +Drupal 9 compatibility

Per a Slack discussion with Gábor Hojtsy regarding usage of D9 tags (Drupal 9, Drupal 9 compatibility, Drupal 9 readiness, etc.), "Drupal 9 compatibility" should be used for contributed projects that need updating and "Drupal 9" was the old tag for D8 issues before the D9 branch was ready. Doing tag cleanup here based on that discussion.