Just wanted to correct a little issue showing up in lint check.

It is better for the static analysis tools to be informed of all possible type for a parameter - especially in the hyper paranoid world of Xss filtering.

It is a one line fix.

CommentFileSizeAuthor
xss-0.patch812 bytesmartin107

Comments

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Reviewed & tested by the community
Issue tags: +Quickfix

Fair

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/editor/src/EditorXssFilterInterface.php
@@ -34,7 +34,7 @@
+   * @param \Drupal\filter\FilterFormatInterface|null $original_format

I don't think the |null is necessary. (optional) is enough. I think hyper paranoid world is covered by the typehint.

martin107’s picture

martin107 commented

Please forgive me, I disagree

"|null" is something my subconscious brain, seems to look out for.

My imperfect brain reads the current line, perfectly.
( And I don't want to degrade the original intent of #2099741: Protect WYSIWYG Editors from XSS Without Destroying User Data )

I just wanted some tools I use to be more correctly informed ... so I won't be applying the changes.
Closing the issue as won't fix works for me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/modules/editor/src/EditorXssFilterInterface.php
@@ -34,7 +34,7 @@
-   * @param \Drupal\filter\FilterFormatInterface $original_format|null

This is still a docs bug

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#3: I'm also confused why you say that; we do it that way all over core? This is about typehints, and according to http://php.net/manual/en/function.gettype.php, NULL is its own type. A typehint should indicate all possible types. NULL is its own type. So… why not add it?

alexpott’s picture

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

Okay never mind me - I'm wrong. Still then the (optional) looks optional.

alexpott’s picture

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

Docs fixes are not subject to beta evaluation. Committed a160367 and pushed to 8.0.x. Thanks!

  • alexpott committed a160367 on 8.0.x 
    Issue #2418611 by martin107: Trivial fix to EditorXssFilterInterface::...

Status: Fixed » Closed (fixed)

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