Page MenuHomePhorge

Fix the whitespace issue in the message "Access Denied: Restricted File"
ClosedPublic

Authored by roberto.urbani on Aug 23 2023, 07:49.
Tags
Referenced Files
F2235155: D25419.1718925715.diff
Wed, Jun 19, 23:21
Unknown Object (File)
Mon, Jun 17, 23:58
Unknown Object (File)
Sun, Jun 16, 18:14
Unknown Object (File)
Fri, Jun 14, 15:29
Unknown Object (File)
Wed, Jun 12, 05:11
Unknown Object (File)
Mon, Jun 10, 04:28
Unknown Object (File)
Fri, Jun 7, 01:16
Unknown Object (File)
Thu, Jun 6, 11:16

Details

Summary

Added whitespaces where missing in the restricted file message.

Closes T15270

Test Plan

Access a forbidden file. Check whitespaces in the error message.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Updating D25419: Fix the whitespace issue in the message "Access Denied: Restricted File"

Updating D25419: Fix the whitespace issue in the message "Access Denied: Restricted File"

Fixed a linting issue, the line was too long so the build was failing

Thanks. I would like to help in reviewing but I don't get the difference. Can you describe the inline change?

0. Welcome!

  1. This diff is based of a commit that's not in master - some local changes that make it not-apply
  2. the space should be on the inside of the pht(), not attached to it.
  3. even better, find whatever is calling describeAutomaticCapability(), and if it accepts an array (some implementations return array, some return string), have the caller add a space between each element of the array - solving this once for all callers...

It seems your "master" branch has (many?) local unexpected commits.

In order to work on this feature, in Phorge, try these:

git status
# you should be on master - please eventually manually delete any local uncommitted change

Then, create another clean branch:

git branch   pippo_feature
git switch   pippo_feature
git reset --hard origin/master

So now you are on a custom branch pippo_feature that is super-clean. From there you can add fresh features and propose whatever patch.

(To switch between branches, just git switch master or git switch pippo_feature ecc.)

This revision now requires changes to proceed.Aug 28 2023, 08:19

Updating D25419: Fix the whitespace issue in the message "Access Denied: Restricted File"

Adding spaces directly where describeAutomaticCapability is called

It seems your "master" branch has (many?) local unexpected commits.

In order to work on this feature, in Phorge, try these:

git status
# you should be on master - please eventually manually delete any local uncommitted change

Then, create another clean branch:

git branch   pippo_feature
git switch   pippo_feature
git reset --hard origin/master

So now you are on a custom branch pippo_feature that is super-clean. From there you can add fresh features and propose whatever patch.

(To switch between branches, just git switch master or git switch pippo_feature ecc.)

Thanks a lot for the workflow, pippo example is always the best example

What do you think about the related tip? Also here I can maybe try to propose a counter-patch next week

Kindly marking as "request changes" only to indicate "waiting for author feedback"

This revision now requires changes to proceed.Sep 18 2023, 06:05

@valerio.bozzolan I actually think the right place to look at the usage of getSpecialRules() might be here: https://we.phorge.it/source/phorge/browse/master/src/applications/policy/filter/PhabricatorPolicyFilter.php;4f845d8f8c77$693-719

I don't know exactly what to change there, though. I see the $text_details = implode(' ', $text_details); which seems to be doing the right thing, but that variable is then used in the exception's full message, whereas the $html_details is constructed as merely array($head, $more, $exceptions); and then used as setMoreInfo($html_details);.

I see also the usage of this PhabricatorPolicyException::setMoreInfo() sets the moreInfo class member which is then read by PhabricatorPolicyException::getMoreInfo(), in particular here: https://we.phorge.it/source/phorge/browse/master/src/aphront/handler/PhabricatorPolicyRequestExceptionHandler.php;9481b9eff111$58-84 — which is weird, because the AphrontDialogView::appendList() method, which is called there, appears to produce an <ul> tag, as per the code in defined in https://we.phorge.it/source/phorge/browse/master/src/view/AphrontDialogView.php;a89b4ff5b8bc$226-243. So I'm a bit lost at this point...

... whereas the $html_details is constructed as merely array($head, $more, $exceptions); ...

I suspect that was/is there to allow $exceptions to have fancy HTML content, but if there is any fancy HTML content in there, the result of $text_details = implode(' ', $text_details) would end up strange (and be text anyway. and maybe start throwing/warning in php 8?). I wish we had more static typing.

..., appears to produce an <ul> tag, as per the code...

The value of moreInfo here is a list (array) of 3 things, (https://we.phorge.it/source/phorge/browse/master/src/applications/policy/filter/PhabricatorPolicyFilter.php;4f845d8f8c77$702) - $head, $more and $exceptions, and I'm guessing from context that the appendList() creates one <ul> for the list and one <li> for each item in the list - so $exceptions at this point is a single thing.

Anyway, I think it would be ok to assume that getSpecialRules should return a "list of strings", and then either implode it in habricatorPolicyFilter, or go with the added whitespace as currently suggested here.

src/applications/policy/storage/PhabricatorPolicy.php
487 ↗(On Diff #1334)

From a code-style pov, each argument should be separated to its own line(s):

$exceptions = array_map(
  function ($item) {
    return $item.' ';
  },
  $exceptions);

The value of moreInfo here is a list (array) of 3 things, (https://we.phorge.it/source/phorge/browse/master/src/applications/policy/filter/PhabricatorPolicyFilter.php;4f845d8f8c77$702) - $head, $more and $exceptions, and I'm guessing from context that the appendList() creates one <ul> for the list and one <li> for each item in the list - so $exceptions at this point is a single thing.

Oh! Yeah, that makes sense, thanks for the clarification!

Anyway, I think it would be ok to assume that getSpecialRules should return a "list of strings", and then either implode it in habricatorPolicyFilter, or go with the added whitespace as currently suggested here.

So would the latter option mean doing $html_details = array($head, $more, implode(' ', $exceptions)); here? I kind of find this approach cleaner than appending a space to each item in the $exceptions array.

So would the latter option mean doing $html_details = array($head, $more, implode(' ', $exceptions)); here? I kind of find this approach cleaner than appending a space to each item in the $exceptions array.

Yeah, exactly.

Should I go ahead and submit a different patch? Or could I somehow contribute the edit to this patch?

Should I go ahead and submit a different patch? Or could I somehow contribute the edit to this patch?

@waldyrious: I think you could ./arcanist/bin/arc patch D25419, make changes, and then ./arcanist/bin/arc diff --update D25419 to push an updated version.
Anyone could always travel back in time via arc patch --diff 1334.

(In the meanwhile, removing the red semaphore, since the goal was to attract reviewers and it worked asd)

  • Add spaces in HTML message builder instead of to each item text

Thanks for the note, Andre! I've went ahead and updated the revision as discussed above.

I can confirm that this patch fixes the string problem. However, I am super super puzzled that after applying this patch, the CSS definition line-height: 22px for .phabricator-wordmark is suddenly missing in both Chromium 120 and Firefox 120, moving the wordmark a bit higher in the upper left corner (on my two screenshots, that is "Phoriorge").

Screenshot from 2023-12-19 00-32-46.png (682×761 px, 87 KB)

Screenshot from 2023-12-19 00-32-52.png (682×761 px, 85 KB)

That's so weird! My instinct is of course to say that it should be completely unrelated to this patch since no CSS is touched in this one-line change... I have no idea how to debug this ☹️

I believe I had some weird aggressive local browser cache pollution with rPaa8af1d79e8bfeb09e72d5e3b9330780e78b7aeb as I cannot reproduce the wordmark problem anymore, so: LGTM

This revision is now accepted and ready to land.Jan 14 2024, 09:13

I'm just unsure between implode() or phutil_implode_html()

In my understanding all these strings should be plain text (no hypertext involved).

On the other hand, as the variable is called $html_details let's go for phutil_implode_html() to be super-safe? ping @roberto.urbani

aklapper requested changes to this revision.May 5 2024, 09:55

(Per my last comment)

This revision now requires changes to proceed.May 5 2024, 09:55
  • Use phutil_implode_html() instead of implode()

Thanks for the recommendation, Andre! I conferred with @valerio.bozzolan and he agrees, too. I have updated the patch.

Below are the screenshots of the result:

Before:

image.png (322×359 px, 29 KB)

After:
image.png (322×359 px, 30 KB)

Thanks to @waldyrious for the change. Tested. It works.

\o/

lgtm

This revision is now accepted and ready to land.May 5 2024, 11:01