Problem/Motivation

PHPstan is giving a warning when using the editor_load function.
Parameter #1 $format_id of function editor_load expects int, string given.

When creating a text-format the machine name can be anything, not just an int.

Steps to reproduce

Run phpstan when using the function editor_load()

Proposed resolution

Change the expected argument to int|string

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3409040.patch514 bytessidharth_soman

Issue fork drupal-3409040

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

robertragas created an issue. See original summary.

sidharth_soman’s picture

Status: Active » Needs review
StatusFileSize
new514 bytes

I have modified the argument's comment. Apply against 10.0.x branch.

cilefen’s picture

Version: 10.0.x-dev » 11.x-dev
Issue tags: +Documentation
smustgrave’s picture

Status: Needs review » Needs work

We should see what's calling it with a string and determine if that's incorrect.

Also would have to look at when that typehint was added if there was discussion about int vs string, which should be documented.

wim leers’s picture

Title: editor_load issue - phpstan expects int » editor_load() typehint is wrong: $format_id is integer, not string
Status: Needs work » Reviewed & tested by the community

Actually, this seems like a very obvious, trivial mistake. Text formats had numerical IDs in Drupal 7. They don't in Drupal >=8.

No further research necessary IMO.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

wim leers’s picture

Issue tags: +Needs reroll

Keshav Patel made their first commit to this issue’s fork.

keshav patel’s picture

Status: Needs work » Needs review

Patch Rerolled and all checks passed. please review.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Reroll seems fine

longwave’s picture

Is it ever really int any more? Can we set it to string only?

smustgrave’s picture

Status: Reviewed & tested by the community » Needs review

Will try and take a deeper look tomorrow

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

So looking into editor_filter_xss() there's a line

$editor = $format ? editor_load($format->id()) : NULL;

and id() can actually return string|int|null so guess int can still be passed.

catch’s picture

Status: Reviewed & tested by the community » Needs work

That looks like a bug in editor_filter_xss(), it should check isNew() before trying to load. Or if not, it should specify string|int|null. But I think we should just type hint on string here.

shalini_jha made their first commit to this issue’s fork.

shalini_jha’s picture

Status: Needs work » Needs review

I Have addressed the feedback, moving this NR. kindly review.

smustgrave’s picture

Status: Needs review » Needs work

Doesn't seem to address the feedback from #15.

Think the comments were missed when converting to an MR.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.