Paths

Table of Contentst

Differential D25415

Make InterpreterBlockRule regex only match on valid interpreter names
ClosedPublic
Actions

Authored by aklapper on Aug 19 2023, 18:22.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15372: "RuntimeException: Undefined array key" when pasting "a {{{a}}} a" comment (due to regex in PhutilRemarkupInterpreterBlockRule.php)

Commits

rP7868ab3754fa: Make InterpreterBlockRule regex only match on valid interpreter names

Summary

With this patch, the underlying exception described in T15372#8537 still remains. However, with this patch, the bug is more contained as it is not triggered when not calling an interpreter (cowsay, figlet), so Phorge does not crash rendering noValidInterpreter {{{foo}}} bar lines but renders them as is (for whatever reasons such lines may exist).

See T15372

Test Plan

Enter strings into a comment:

invalid {{{saysay}}} foo now renders as plain text instead of crashing
invalid (invalid) {{{saysay}}} foo now renders as plain text instead of crashing
cowsay (invalid) {{{saysay}}} foo will still crash as before

Diff Detail

Repository

rP Phorge

Branch

D25234aug (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 825
Build 825: arc lint + arc unit

Event Timeline

aklapper created this revision.Aug 19 2023, 18:22

Owners added a reviewer: O1: Blessed Committers.Aug 19 2023, 18:22

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptAug 19 2023, 18:22

aklapper requested review of this revision.Aug 19 2023, 18:22

Harbormaster completed remote builds in B790: Diff 1313.Aug 19 2023, 18:22

aklapper mentioned this in T15372: "RuntimeException: Undefined array key" when pasting "a {{{a}}} a" comment (due to regex in PhutilRemarkupInterpreterBlockRule.php).Aug 19 2023, 18:22

aklapper edited the test plan for this revision. (Show Details)Aug 19 2023, 18:28

This is very promising. Just added an inline comment

src/infrastructure/markup/blockrule/PhutilRemarkupInterpreterBlockRule.php
25–32	We can maybe try to avoid `end($array)` since it's supposed to modify the internal pointer of the array and here it works as expected only for truly dark magic from advanced PHP . Also we can maybe call `preg_quote()` over each elements in order to sanitize the input before building the regex. Also it's probably nice to just use `implode()` for concatenation. Also interestingly we have a "method pull" function called `mpull()` that just creates an array with a function call. Result: $interpreter_names_regex = mpull($interpreters, 'getInterpreterName'); $interpreter_names_regex = array_map('preg_quote', $interpreter_names_regex); $interpreter_names_regex = implode('\|', $interpreter_names_regex); Do you somehow like this?

valerio.bozzolan awarded a token.Aug 29 2023, 06:57

Simplify code with neat functions as proposed by Valerio

Harbormaster completed remote builds in B824: Diff 1360.Sep 2 2023, 11:31

Shorten too long line

Harbormaster completed remote builds in B825: Diff 1361.Sep 2 2023, 11:33

aklapper marked an inline comment as done.Sep 2 2023, 11:34

Nice catch. I agree that we should allow to have texts like asd {{{ lol }}} whey without any nuclear implosion needed by design just because we are trying to give support to cowsay or figlet.

I tested and this seems good to me.

For the records, at the moment we have 3 block interpreters:

cowsay
figlet
phutil_test_block_interpreter (!)

I was not aware of the last one. Trying that:

phutil_test_block_interpreter (asd=ASD,lol) {{{ EHEHEHEH }}}

It renders as (the blockquote is mine):

Content: ( EHEHEHEH ) Argv: (asd=ASD&lol=1)

What a super-interesting new feature we discovered! Awesome. Now we can concentrate on the root cause of:

T15372: "RuntimeException: Undefined array key" when pasting "a {{{a}}} a" comment (due to regex in PhutilRemarkupInterpreterBlockRule.php)

src/infrastructure/markup/blockrule/PhutilRemarkupInterpreterBlockRule.php
21–23	✅ From PhutilClassMapQuery: «This class automatically caches class maps and does not need to be wrapped in caching logic.»

This revision is now accepted and ready to land.Sep 4 2023, 10:51

Closed by commit rP7868ab3754fa: Make InterpreterBlockRule regex only match on valid interpreter names. · Explain WhySep 4 2023, 11:12

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rP7868ab3754fa: Make InterpreterBlockRule regex only match on valid interpreter names.

valerio.bozzolan mentioned this in Z1: Phorge.Mar 19 2024, 17:00

valerio.bozzolan added a task: T15372: "RuntimeException: Undefined array key" when pasting "a {{{a}}} a" comment (due to regex in PhutilRemarkupInterpreterBlockRule.php).Mar 25 2024, 07:12

valerio.bozzolan mentioned this in D25559: Update unit test of InterpreterBlockRule for "a {{{a}}}".Mar 25 2024, 07:18

valerio.bozzolan mentioned this in rP9ebe85621919: Update unit test of InterpreterBlockRule for "a {{{a}}}".Mar 25 2024, 08:28

Revision Contents
Changeset List

Path

Size

Packages

src/

infrastructure/

markup/

blockrule/

PhutilRemarkupInterpreterBlockRule.php

30 lines

O1: Blessed Committers

Diff 1361

View Options

src/infrastructure/markup/blockrule/PhutilRemarkupInterpreterBlockRule.php

Make InterpreterBlockRule regex only match on valid interpreter namesClosedPublicActions