check_markup(): $format_id must be required, $langcode should default to LANGCODE_NOT_SPECIFIED

For context, passing language to filters was introduced in #319788: Pass language information to filters.

all code totally has to pass a $langcode to check_markup(), so all filters can safely rely on it.

sun, could you expand? Why? Is passing a code like FIELD_LANGUAGE_NONE an option when the language isn't known?

Some objects that we check markup for just don't have a language, even that of a parent. Blocks would be one: block_block_view(). So it's hard to see how we would pass a meaningful language code in every case.

Blargh.

Now that #2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags has landed, this will actually be quite trivial fix.

At all places, where check_markup() is called without a $langcode, are those supposed to be fixed here only or that'll require a follow-up?

Since there are only a dozen calls to check_markup() left, I'd say it can and should happen in this issue.

Couple of things to notice:

1. Function signature for check_markup() has changed. Now $langcode appears before $format_id, which is still optional while $langcode is required. Since its an API change, it may require a change notice.
2. Places where check_markup() was called without a language code, in calls to check_markup() an empty string is used currently.

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
@@ -325,7 +325,7 @@ protected function generateFormatTagsSetting(Editor $editor) {
+        $output = trim(check_markup($input, '', $editor->id()));

This should be Drupal\Core\Language::LANGCODE_NOT_SPECIFIED. Also elsewhere where the empty string is used.

#14: I couldn't agree more! :)

In fact, I was writing a tweet to you just now, sun, to ask you about precisely that :D

@Wim: In cases where langcode is not specified, should i include Drupal\Core\Language or use the fully qualified name for LANGCODE_NOT_SPECIFIED?

Changes from patch in #10:

1. Order of parameters retained.
2. Made $format_id required.
3. Pass NULL in places where $format_id was not passed earlier.
4. Pass LanguageInterface::LANGCODE_NOT_SPECIFIED in places where $langcode was either not specified earlier or empty string was used.

OK, now I'm a bit confused about what we're trying to do here. From what I understand if any format_id is not specified, same way as text in the fallback format. And that's precisely why this test is in place: Drupal\filter\Tests\FilterNoFormatTest.

  /**                                                                                                                                                 
   * Tests text without format.                                                                                                                       
   *                                                                                                                                                  
   * Tests if text with no format is filtered the same way as text in the                                                                             
   * fallback format.                                                                                                                                 
   */                                                                                                                                                 
  function testCheckMarkupNoFormat() {                                                                                                                
    // Create some text. Include some HTML and line breaks, so we get a good                                                                          
    // test of the filtering that is applied to it.                                                                                                   
    $text = "<strong>" . $this->randomName(32) . "</strong>\n\n<div>" . $this->randomName(32) . "</div>";                                             
                                                                                                                                                      
    // Make sure that when this text is run through check_markup() with no text                                                                       
    // format, it is filtered as though it is in the fallback format.                                                                                 
    $this->assertEqual(check_markup($text, NULL, LanguageInterface::LANGCODE_NOT_SPECIFIED), check_markup($text, filter_fallback_format(), LanguageInt
  }                                                                                                                                                   

On the other hand, there's no such test in place for default langcode. Something is wrong here.

I'm still not sure whether we need to make the $langcode parameter required, or simply make it default to LanguageInterface::LANGCODE_NOT_SPECIFIED. It sounds perfectly acceptable to me to not pass a language code into check_markup() — input filters will simply produce language-agnostic output. If you expected language-aware output, then you should pass an explicit language code.

+1

Adjusting title accordingly.

Someone needs to rewrite the issue summary what we really want (refer to @sun's comment in #23).

Patch with following changes:

1. Function changed as per #20.
2. Removed LanguageInterface::LANGCODE_NOT_SPECIFIED from calls to check_markup() where it was not required.

Let's see if testbot agrees.

Uploading the interdiff in correct format.

Fixed the issues in #30.

@sun: Is anything else required here?

We need a change record for this one.

Drafted change record: https://www.drupal.org/node/2305883

+++ b/core/modules/filter/filter.module
@@ -463,7 +465,7 @@ function filter_fallback_format() {
-function check_markup($text, $format_id = NULL, $langcode = '', $filter_types_to_skip = array()) {
+function check_markup($text, $format_id, $langcode = LanguageInterface::LANGCODE_NOT_SPECIFIED, $filter_types_to_skip = array()) {

I don't understand what is accomplished here. Because of the move to '#type' => 'processed_text', filter_pre_render_text() still allows for a NULL $format_id, and we don't save any code.

#37: It's indeed possible to still set the format ID to NULL if you really want to; but the public API should really force you to set an actual format. That's what this is about.
However, if you're saying that the default of '#format' => NULL should be removed from filter_element_info(), then I agree. And if we do that, we'll also want to remove this from filter_pre_render_text():

  if (!isset($format_id)) {
    $format_id = filter_fallback_format();
  }

And perhaps instead of that, we'll want to throw an exception if '#format' is not set.

I think we should do both of the things mentioned in #38, that's what I had in mind.

Tagging Amsterdam sprint.

filter_pre_render_text() was moved in commit 2c1fe04, but it still seems that the equivalent code block should be removed from core/modules/filter/src/Element/ProcessedText.php (where the code was moved to).

For (an) experienced Drupal/PHP coder but novice contributor(s):

- understand the issue generally
- check what is mentioned in #38/#39
- check my assumption
- do the work
- review the work
- check if this makes changes to the Change Record draft necessary.

(Fix wrong paste of 'contributor tasks' table in summary)

We will have a look at this...

Here's a patch that implements the previous patch #31, plus Wim's comments in #38. hctom is working on tests.

Here is a patch that implements previous patches #31 and #43 and adds more changes:

• Changed the documentation of check_markup() to reflect required $format_id argument
• Improved documentation of Drupal\filter\Element\ProcessedText to name the new exception thats thrown when no $format_id is passed
• Changed Drupal\filter\Tests\FilterNoFormatTest to check exception throwing when no $format_id is passed
• Changed Drupal\views\Tests\Handler\AreaTextTest to not use check_plain($string, NULL) but check_plain($string, filter_fallback_format())

Attaching interdiff

Can you re-upload the patch in #44? Testbot is not detecting it because you attached new files while setting the status to NR. Thanks :)

Here you go.

After doing a rebase I recreated the patch of #44. See this comment for all applied changes.

A Format ID is not set exception is thrown, so the test coverage is uncovering another case where check_markup() is being called with $format_id = NULL :)

Should be an easy fix!

Unfortunately this is not such an easy fix, because there are several tests that rely on #format = NULL (e.g. Drupal\text\Tests\TextWithSummaryItemTest). So I now only additionally added filter_fallback_format() to all dynamic #format values and would like to hand over the the checking of the other tests to people who are more deep into the materia, because I do not want to supersede those tests by eventually making the wrong changes to them.

Attached you can find a patch containing all my recent changes as well as of course all changes from #31, #43 and #44.

Attached you can find 2 interdiffs. Hope these help?!

re-rolled

I am removing the Novice tag from this issue because there are no easy action items for this task.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

This was an issue at a bugsmash group triage. After reading the comments and looking at the patch and the existing code, this appear to still be relevant. This does need an Issue Summary update. And it needs tests, see #52 and #53.