Problem/Motivation

The "Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent::getException()" method is deprecated since Symfony 4.4, use "getThrowable()" instead

See https://www.drupal.org/pift-ci-job/1622423

The whole Symfony class GetResponseForExceptionEvent is deprecated, so we need to replace it everywhere.

Proposed resolution

Replace all GetResponseForExceptionEvent in core at once.

Remaining tasks

Commit patch.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 3113876-15.patch38.4 KBjungle
#11 interdiff-8-11.txt1.25 KBhardik_patel_12
#11 3113876-11.patch9.16 KBhardik_patel_12
#8 interdiff-4-8.txt5.65 KBhardik_patel_12
#8 3113876-8.patch7.96 KBhardik_patel_12
#5 3113876-4.patch2.52 KBhimmatbhatia
#5 interdiff_3-4.txt689 byteshimmatbhatia
#3 13466790-3.patch2.19 KBhimmatbhatia

Comments

himmatbhatia created an issue. See original summary.

himmatbhatia’s picture

himmatbhatia commented
Assigned: Unassigned » himmatbhatia
himmatbhatia’s picture

himmatbhatia commented
Assigned: himmatbhatia » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 13466790-3.patch2.19 KB

I have fixed the problem and attaching the patch file.

Status: Needs review » Needs work

The last submitted patch, 3: 13466790-3.patch, failed testing. View results

himmatbhatia’s picture

himmatbhatia commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_3-4.txt689 bytes
new 3113876-4.patch2.52 KB

One place was missed so changed that place as well and attaching patch

Status: Needs review » Needs work

The last submitted patch, 5: 3113876-4.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The fails here make me think that we should just be replacing all these in one go, rather than module by module.

hardik_patel_12’s picture

hardik_patel_12 commented
StatusFileSize
new 3113876-8.patch7.96 KB
new interdiff-4-8.txt5.65 KB

Kindly review a new patch.

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 3113876-8.patch, failed testing. View results

hardik_patel_12’s picture

hardik_patel_12 commented
StatusFileSize
new 3113876-11.patch9.16 KB
new interdiff-8-11.txt1.25 KB

Kindly review a new patch.

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

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

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Title: Parameter $event of method Drupal\jsonapi\EventSubscriber\DefaultExceptionSubscriber::onException() has typehint with deprecated » The "Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent::getException()" method is deprecated since Symfony 4.4, use "getThrowable()" instead
Component: jsonapi.module » base system
Assigned: Unassigned » jungle
Issue summary: View changes

Agree with @longwave, let's do it in one go

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs work » Needs review
StatusFileSize
new 3113876-15.patch38.4 KB
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Assigned: jungle » Unassigned
Related issues: +#3094398: Update Symfony response and request events to new classes

duplicated probably

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

#3094398 did two types of fixes. From the point of scope, maybe this one should be kept.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Parent issue: » #3055180: [META] Symfony 5 compatibility
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Nice thanks!

I checked manually that there are no remaining GetResponseForExceptionEvent references in core.

himmatbhatia’s picture

himmatbhatia commented
catch’s picture

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

Committed a5fb386 and pushed to 9.1.x. Thanks!

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

@catch. Thank you for committing! One more question, Should it be committed to 9.0.x as well?

  • xjm committed 5c457b5 on 9.1.x 
    Issue #3113876 by Hardik_Patel_12, himmatbhatia, jungle, klausi,...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 9.0.x-dev » 9.1.x-dev

It looks like this might not have been pushed to 9.1.x. Since it's a 40K patch that could end up needing rerolls and since it's not working hours where @catch is, I recommitted the patch. (I have not reviewed it myself.)

I wouldn't backport this to 9.0.x because there are some potentially disruptive changes in it (changed typehints etc.). Since it's event subscribers and so forth it's an internal change, but probably not recommended during beta. So, setting to 9.1.x unless @catch has other thoughts.

  • catch committed d808600 on 9.0.x 
    Issue #3113876 by Hardik_Patel_12, himmatbhatia, jungle, longwave,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.1.x-dev » 9.0.x-dev

Sorry folks I got distracted and didn't push the commits last night.

I do think this is OK to backport to 9.0.x, it will help contrib modules to not have the deprecation message suppressed and every change is @internal. Already did that last night, but also pushed the commit this morning.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Thanks, @catch and @xjm! looking forward to #3114122: ExceptionLoggingSubscriber should log 403 access denied reason being landed soon, which was blocked by this one.

Status: Fixed » Closed (fixed)

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

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Found this issue from an Upgrade Status report. While I understand this is out of the supported API realm, if someone has bright ideas to help cover #3131063: Detect GetResponseForExceptionEvent is deprecated in favour of ExceptionEvent that would be useful nonetheless.