Added whitespaces where missing in the restricted file message
Details
- Reviewers
valerio.bozzolan - Group Reviewers
O1: Blessed Committers
access a forbidden file to check the whitespaces
Task ref: T15270
Diff Detail
- Repository
- rP Phorge
- Branch
- arcpatch-D25419
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 808 Build 808: arc lint + arc unit
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!
- This diff is based of a commit that's not in master - some local changes that make it not-apply
- the space should be on the inside of the pht(), not attached to it.
- 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.)
Updating D25419: Fix the whitespace issue in the message "Access Denied: Restricted File"
Adding spaces directly where describeAutomaticCapability is called
Nice catch
I'm not 100% sure but maybe it's the consumer of getSpecialRules() that should do that.
Maybe here:
What do you think about?
What do you think about the related tip? Also here I can maybe try to propose a counter-patch next week
@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 | From a code-style pov, each argument should be separated to its own line(s): $exceptions = array_map( function ($item) { return $item.' '; }, $exceptions); |
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?