`core/modules/jsonapi/src/Context/FieldResolver.php` file containing \Drupal calls which should be passed as dependency injection. or might in other places it should be corrected in jsonapi module.

CommentFileSizeAuthor
#12 3147244-12.patch3.83 KBbbrala
#12 interdiff-5-12.txt3.42 KBbbrala
#5 3147244-5.patch3.74 KBpavnish
#5 interdiff_2_5.txt2.77 KBpavnish
#2 3147244-2.patch3.06 KBnaresh_bavaskar

Comments

naresh_bavaskar created an issue. See original summary.

naresh_bavaskar’s picture

naresh_bavaskar
Location Pune, Maharashtra
commented
Assigned: naresh_bavaskar » Unassigned
StatusFileSize
new 3147244-2.patch3.06 KB

Status: Needs review » Needs work

The last submitted patch, 2: 3147244-2.patch, failed testing. View results

pavnish’s picture

pavnish commented
Assigned: Unassigned » pavnish

Working on it

pavnish’s picture

pavnish commented
Assigned: pavnish » Unassigned
Status: Needs work » Needs review
StatusFileSize
new interdiff_2_5.txt2.77 KB
new 3147244-5.patch3.74 KB

@naresh_bavaska found some wrong code in #2
Please review this patch.

naresh_bavaskar’s picture

naresh_bavaskar
Location Pune, Maharashtra
commented

@pavnish, Thanks for correcting the code, LGTM, +RTBC

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Add Dependancy injection in php files where needed in JSON API module » Inject @current_user service into JSON:API's FieldResolver
Status: Needs review » Reviewed & tested by the community

I really wondered why this was the case, so I dug in and … ahhh … that's because it was introduced in https://git.drupalcode.org/project/jsonapi/-/commit/30559bfaf5055505fad4..., for a security issue. To minimize disruption, it was decided by the security team to not use dependency injection.

Nice catch 👍

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

And nice collaboration 😊

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

So this class is marked @internal but it is extended and the service decorated in https://www.drupal.org/project/jsonapi_cross_bundles - I don't think this will break because of the way the decoration and class extension is done. But it would be good to confirm that this is the case.

A more traditional way of adding an argument is allowing it to be NULL and then triggering a deprecation if it is not supplied. I think even though this is marked as @internal we should make best effort to not break contrib / custom code so should consider doing the deprecation dance here. We'll get to require the argument in D10.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bbrala’s picture

bbrala
Location Netherlands
commented
StatusFileSize
new interdiff-5-12.txt3.42 KB
new 3147244-12.patch3.83 KB

As requested @alexpott, the deprecation dance. ^^

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review
wim leers’s picture

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

Can't spot any problem! 🤓

bradjones1’s picture

bradjones1
Primary language English
commented

I had a question about whether this should be a nullable parameter type but TIL that = null in a parameter implies ?.

bbrala’s picture

bbrala
Location Netherlands
commented

To be honest, I double checked core for examples of this construct to be sure its supported :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

@bradjones1 IIRC Something $something = NULL predates nullable types :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed f928fe9 and pushed to 9.3.x. Thanks!

  • alexpott committed f928fe9 on 9.3.x 
    Issue #3147244 by bbrala, pavnish, naresh_bavaskar, Wim Leers, alexpott...

Status: Fixed » Closed (fixed)

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