Filter placeholders without arguments are not replaced when HTML corrector filter applied afterwards

Switching to target Drupal 11.

MR includes a fix and updated tests to demonstrate the issue. The tests without the fix fail as expected (although I'm surprised to see GitLab now shows that with a green tick! But the detailed output shows the failure), and with the fix they pass as expected.

Small super nitpicky change

But MR should be pointed to 11.x vs 11.0.x as 11.x is still the "main" branch.

I see the test-only feature passes when it should fail so tests seem to need some work.

As far as your comment on the MR I also could not find a spot and we don't have a filter sub-maintainer so think it's fine for now.

Thanks for the review. As I mentioned before, the detailed output in the log of the test-only feature shows there was a test failure. I don't know why the pipeline makes it look like it succeeded!

Ah @fjgarlin has helped clarify on slack that the tests-only feature showing as passed is a bug in core's test-only script. Please see from the detailed log that the test really does fail!

james.williams → closed merge request !8392

The fact that the tests-only run can 'pass' is handled in 🐛 Test-only job does not detect failures correctly Needs review (which I'm having a go at progressing). I'm including the change from there here for now, as advised by @fjgarlin there.

If there are no arguments, would it be better/more correct to not output the arguments attribute at all? ie. we would just have <drupal-filter-placeholder callback="callback" token="token" />? Then, the HTML5 parser/emitter does not need to special case this at all.

In PlaceholderGenerator we did update it to optionally output the arguments for HTML5, so I do think we should try the same here:

    $placeholder_markup = '<drupal-render-placeholder callback="' . Html::escape($callback) . '"';
    if ($arguments !== '') {
      $placeholder_markup .= ' arguments="' . Html::escape($arguments) . '"';
    }
    $placeholder_markup .= ' token="' . Html::escape($token) . '"></drupal-render-placeholder>';

Pushed a change for #11.

Oh nice idea, thank you! Yes, I'm on board with that, I've tested my case manually too just to check, and it works fine. Combined with the automated test, I trust it's OK to put this to RTBC based on that.

If I were being really nitpicky, I'd suggest we check Html::escape($arguments) !== '', since it's the escaped version that gets used, but I see we're just doing the same as in PlaceholderGenerator::createPlaceholder(). So if that nitpick even mattered, we could just change both in a follow-up. But I'm not being nitpicky, so ignore that thought ;-)

I think in a followup we could even refactor that code into a static helper method on PlaceholderGenerator, so all placeholders are constructed in a consistent way.

Mistakenly removed the original tag when working on this at Dev Days.

I read the IS, comments and MR. There are no outstanding questions and I had no recommendations to make on the MR. Leaving at RTBC

Added tag for a followup for #15.

Committed to 11.x and backported to 11.0.x, 10.4.x and 10.3.x

Thanks folks

larowlan → closed merge request !8465

Woah, thank you!

Opened 📌 Add helper method to generate HTML placeholders Active to simplify the code here.

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

Wow, quite the edge case! Introduced by yours truly all the way back in #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache → .