Problem/Motivation

#3254245: TypeError: Argument 1 passed to Drupal\Core\File\FileUrlGenerator::generateString() must be of the type string, null given fixed the bug but we can slightly improve the implementation by using a return typehint (9.3.x+).

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3267078-2-9.3.x.patch617 bytesalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Add return typehint and assert to » Add return typehint to
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3267078-2-9.3.x.patch617 bytes

Adding this is allowed because \Drupal\Core\Template\TwigExtension::getFileUrl() has not yet been released.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#3254245: TypeError: Argument 1 passed to Drupal\Core\File\FileUrlGenerator::generateString() must be of the type string, null given
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Add return typehint to » Add return typehint to TwigExtension::getFileUrl()
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Makes sense to me. RTBC if tests pass.

I'm not sure about changing and enforcing a string in the future. On one side, it is generally a bug if you pass in NULL, as it will result in broken markup, but clearly a lot of people were very confused about the problem and how to find where it is. An exception or so might have more useful info than a fatal error though?

But that's a separate decision anyway.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3267078-2-9.3.x.patch, failed testing. View results

alexpott’s picture

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

Random JS fail-itis... ran all the failed tests locally and they are passing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3267078-2-9.3.x.patch, failed testing. View results

alexpott’s picture

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

wow - it's just not passing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3267078-2-9.3.x.patch, failed testing. View results

alexpott’s picture

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

Well the sqlite test passed... hopefully that's good enough.

  • catch committed d802bf4 on 10.0.x 
    Issue #3267078 by alexpott, Berdir: Add return typehint to...
  • catch committed 31f93e2 on 9.3.x
    Issue #3267078 by alexpott, Berdir: Add return typehint to...
  • catch committed 557fe88 on 9.4.x
    Issue #3267078 by alexpott, Berdir: Add return typehint to...
catch’s picture

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

Committed/pushed to 10.0.x, cherry-picked to 9.4.x and 9.3.x - the second test run passed.

Status: Fixed » Closed (fixed)

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