Tested again. Double-accept.

• Split getting values and rendering meta tags into separate private methods
• Fix description length check

Triple-slam-accept

• Fix the $award_phids thingy to do the $award_phids[$phid] thing
• I prefer to keep if (!$errors) as that part of code is supposed to check for (any) errors

Before and after it throws an exception; after it involves the return command to do so.

follow @valerio.bozzolan what suggested at https://we.phorge.it/D25750#20386.
we barely used pattern search but it indeed was broken.
with this fix, it works.

We can take inspiration from Phame (?)

It's perfect as-is. Just adding last notes since I'm in verbose mode

I thought there was a ->setFormErrors($errors) but it's not the case. Nice as-is. Tested thanks :3

Uhm. Before the change, the method returns void. After the change, the method returns void. So this proposed change would be "to make PHPStan happy" but I expect it will highlight in future versions of PHPStan as well.

Double-slam-accept

Use canResolve() as found by Valerio (so much for my reading skills)

git rebase master (just because we can)

Seems good thanks. It seems we can also have it shorter with canResolve() 🤔

You didn't have Trusted Contributors membership, which provides access to the repo. Try now

In D25750#20406, @aklapper wrote:

Could you try arc land instead of git push?

Could you try arc land instead of git push?

Many thanks for your patience. you're very kind!
but I do have trouble landing this diff.
not sure what I'm doing wrong.

Please evaluate the same change also here to fix the "Pattern Search"

arc lint

I skiped the linting and unit testing because they take time to process. too much for this tiny change

Too lazy, just doing the same as in rP43d3fd9eac6e8e44d8a60110d8ad6f01bb240979: Turn "Required" red after the first attempt

Rebase after merging rP59678094fb777aed3c3eb66e8075c082b3dc393f

Replying to a comment in Z1 from a week ago: When Phorge's curtain doesn't allow someone to access the content, it shows either a 404 ("Access Denied: Restricted Maniphest Task") or the login dialog. In both cases, no OGP meta tags are leaked.

Thaaaaanks, it works

Same results as Valerio after testing more. Also remove unused vars $title and $header_name - they are unused.

Yup. Laaaand

The $password variable is not a string anymore in the line after. I guess it's not much more expensive to check if the string $username is nonempty versus comparing to a boolean value

As temporary Phorge workaround, we may want to document these email keywords rendered as "progress". So we can just manually add backtics (?)

Quick notes:

• For testing code changes, make sure to ./bin/cache purge --caches remarkup
• rPcc44ae32c546: Remove obsolete "setDisableMacros()" on "PhabricatorRemarkupControl" once upon a time existed, could provide a base
  • But how to even realize/pass the info that we are rendering strings in a documentation context?
• PhutilRemarkupRule has a isFlatText($text) method but no setFlatText(true) method
• PhutilRemarkupSimpleTableBlockRule::markupText() is handling our $cell which has the meme/macro name as its only string content
• Afterwards, PhabricatorMemeRemarkupRule:apply() is the next step being called

Thaaaanks. I wonder why $has_password has a dedicated variable, and instead $has_username has not 🤔

Flagging all comments as done (but feel free to share other things)

Thanks again @speck. This is probably useful to answer your performance question:

😐