Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sahil Khambra created an issue. See original summary.

Sahil Khambra’s picture

SafeMarkup::checkPlain is replaced by Html::escape

Sahil Khambra’s picture

Status: Active » Needs review
martin107’s picture

Yes the method is deprecated, so +1 on the idea behind the issue.
Patch looks clean. - great

Looking at the deprecation note

* @deprecated Will be removed before Drupal 9.0.0. Rely on Twig's
* auto-escaping feature, or use the @link theme_render #plain_text @endlink
* key when constructing a render array that contains plain text in order to
* use the renderer's auto-escaping feature. If neither of these are
* possible, \Drupal\Component\Utility\Html::escape() can be used in places
* where explicit escaping is needed.

breaking that down into a decision flow chart.

If we can assume output will always go into a twig template and will be auto escaped then escape() call is not needed?

I see NumericArgument::getTitle() which we override makes that assumption!

It is free to have an "abundance of caution", so for the moment I am leaving it as it...maybe someone else can comment?

My patch attaches a small fix up.... during review I noticed that we override NumericArgument::getTitle() it is not on any interface
so @inheritdoc is incorrect. When I use doxygen I can confirm that the method documentation is incorrectly blank ... so I have added it in.
Please feel free to object to what I have done and commit the patch from #2.

socketwench’s picture

@deprecated Will be removed before Drupal 9.0.0...

These type of deprecations aren't a high priority for me, which is why I haven't gone out of my way to remove them. I have no problems if anyone else wants to go for it though. ^_~

Right now I think there's nothing wrong with an "abundance of caution".

socketwench’s picture

Status: Needs review » Reviewed & tested by the community
joachim’s picture

Title: Replaced deprecated method to new one » Replaced deprecated SafeMarkup::checkPlain with Html::escape
Status: Reviewed & tested by the community » Needs work

>My patch attaches a small fix up.... during review I noticed that we override NumericArgument::getTitle() it is not on any interface

I see it in the parent class Drupal\views\Plugin\views\argument\NumericArgument:

  /**
   * Override for specific title lookups.
   * @return array
   *    Returns all titles, if it's just one title it's an array with one entry.
   */
  public function titleQuery() {
    return $this->value;
  }

Inheritdoc is valid for overrides of a parent class, surely?

martin107’s picture

From https://www.drupal.org/coding-standards/docs#inheritdoc

In a class, if you are overriding or implementing a method from a base class or interface, and the documentation should be exactly the same as the base class/interface method, use the following docblock syntax:

My mistake - overriding or implementing. It one of those instances where doxygen and d8 standards diverge - no biggie.

I am easy - I would be happy to see the patch from #2 go forward.

martin107’s picture

Status: Needs work » Needs review
joachim’s picture

Status: Needs review » Needs work

The old class is still being imported:

> use Drupal\Component\Utility\SafeMarkup;

martin107’s picture

Status: Needs work » Needs review
FileSize
940 bytes

done

Status: Needs review » Needs work

The last submitted patch, 11: replace_checkplain-2785387-11.patch, failed testing.

The last submitted patch, 11: replace_checkplain-2785387-11.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review

socketwench’s picture

Status: Needs review » Postponed

No longer finding any references to SafeMarkup. Thanks Martin!

socketwench’s picture

Status: Postponed » Fixed

Status: Fixed » Closed (fixed)

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