Problem/Motivation

Method "Symfony\Component\HttpKernel\HttpKernelInterface::handle()" will return "Response" as of its next major version.

Steps to reproduce

Proposed resolution

Add the "Response" return type hint.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 3232082-10.patch15.86 KBdaffie
#10 interdiff-3232082-6-10.txt2.73 KBdaffie
#6 3232082-6.patch14.85 KBdaffie
#6 interdiff-3232082-3-6.patch3.72 KBdaffie
#3 3232082-3.patch11.13 KBdaffie

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie commented
Title: [Symfony 6] Add "Response" type hint to methods that implement the method ::handle() » [Symfony 6] Add "Response" type hint to methods that implement the method Symfony\Component\HttpKernel\HttpKernelInterface::handle()
daffie’s picture

daffie commented
Status: Active » Needs review
StatusFileSize
new 3232082-3.patch11.13 KB

The problem for adding this return type hint is that a lot of contrib modules override the method. See: http://grep.xnddx.ru/search?text=public+function+handle%28&filename=. Maybe we need to wait for D10.0 to commit this patch.

Status: Needs review » Needs work

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

catch’s picture

catch
he/him
Primary language English
commented

Looks like we're returning NULL in some places from the test results, should we open another issue to fix those in 9.x?

daffie’s picture

daffie commented
Version: 9.3.x-dev » 10.0.x-dev
Priority: Normal » Critical
Issue tags: +Drupal 10
StatusFileSize
new interdiff-3232082-3-6.patch3.72 KB
new 3232082-6.patch14.85 KB

Looks like we're returning NULL in some places from the test results, should we open another issue to fix those in 9.x?

The problem is that to fix the tests you also have to add the return type hinting. The required test changes are in the interdiff patch file and they will fail. Therefor for this and the overrides in contrib, I am moving this to 10.0.

This issue is part of the Symfony 6 stuff and that is all critical.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 10.0.x-dev » 9.3.x-dev
Status: Needs work » Needs review

For this to be problematic to contrib, they have to both implement this method *and* extend one of our classes. From a random sample of the results in #3 they either implement HttpKernelInterface directly (in which case they also need to add return types, but it doesn't stop us from doing so beforehand) or are false positives (the method signature is different). I didn't see anything that extends Drupal core code directly.

Therefore moving this back to 9.x for review.

The last submitted patch, 6: interdiff-3232082-3-6.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 6: 3232082-6.patch, failed testing. View results

daffie’s picture

daffie commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3232082-6-10.txt2.73 KB
new 3232082-10.patch15.86 KB

Fixed the testbot failures.

Status: Needs review » Needs work

The last submitted patch, 10: 3232082-10.patch, failed testing. View results

daffie’s picture

daffie commented
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs change record updates

Committed e147702 and pushed to 9.3.x. Thanks!

Thanks for checking contrib and documenting your checks @longwave.

I think we might need a single CR to collect all the SF6 preparations together?

  • alexpott committed e147702 on 9.3.x 
    Issue #3232082 by daffie, longwave, catch: [Symfony 6] Add "Response"...
catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -Needs change record updates

There's a CR here: https://www.drupal.org/node/3236232

I've added this issue.

Status: Fixed » Closed (fixed)

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