Rephrase comment

In D25450#12945, @speck wrote:

Maybe add some doc - the view and edit actions being lumped together are because it would be a larger change to split out that functionality right now, correct?

Show 404 error instead of policy based message

Instead of adding a checkbox is there precedent for having a separate button alongside Cancel and Submit?

I’m surprised the monograms weren’t already defined on the applications - those should be somewhere already, right?

Basically it works

😂

This looks good and I really like the idea of being able to customize the style of external links. Just one tweak to the logic I think we should add before landing.

Looks good to me, I suggest clarifying the comment before landing.

Maybe add some doc - the view and edit actions being lumped together are because it would be a larger change to split out that functionality right now, correct?

Hoping to be useful, I will land this after 2023-11-07 :) Thanks for this nice feature

Hoping to be useful, I will land this in 3 days :) since I love this feature

Basically it works

Ah! Indeed it would be lovely

An example of the TOC in action is in the diviner pages.

Where I can see the TOC in action? I think that I do not see that even here without ?as=remarkup:

However, it would make sense not to query transactions in applications which once were installed and are now uninstalled and not accessible anymore anyway.
In downstream https://phabricator.wikimedia.org/T344232 , the exception is Query (of class "ConpherenceTransactionQuery") overheated.
However we uninstalled Conpherence more than four years ago in https://phabricator.wikimedia.org/T127640 (but of course the DB data is still around).

Create a dedicated method getRemarkupLinkClass() to write that once

fix test

Integrate some tips. Clarify that this is just a compromise between performance and a good default, only to be used for target="_blank" purposes, to don't slow down Remarkup.

Hoping to be useful, implement tips, to be easily tested.

I am sorry, thanks for your last comment, I now understand your point. Yes, I guess we'd better return new Aphront404Response() instead of some permission based message. Like we'd do trying to access the URL of an uninstalled application. Now just need to find out how to do that :)

Thanks for clarifying this behavior, it sounds like contact numbers in general need fleshed out quite a bit.

In D25452#12901, @speck wrote:

I’m guessing the user would be unable to remove their contact number (or even see it), unless turning off sms deletes their contact number. In this case I think the user would still want to know it’s saved somewhere and could remove it.

Changing policy based on sms being configured seems a little off to me. Having the setting only conditionally show based on it being configured seems fine, however what happens in this scenario:

1. Turn on sms
2. Add number
3. Turn off sms

you just adopted PHP 7.4 or something similar I think to fix

Thanks for reporting

Nevermind, this is a problem with my setup, sorry for the noise. (I missed the second line, and of course I realized it right after submitting)

make linter happy

Note that this patch does NOT default to admin as proposed in T15443#9918 as I could imagine confusion by overwriting that value on existing installations. Happy to adjust though...

In P13#202, @smith wrote:

Shouldn't Create and Edit be lowercase?

I don't have much of an attention span these days, sorry.

I had communicated these upstream (almost exactly a year ago~) and some helpful information was provided

Partially related to

I've done my best to implement all proposed improvements

Before this change NULL was indeed implicitly casted to ''