Details

Severity	Location	Code	Message
Warning	src/applications/files/transform/PhabricatorFileImageTransform.php:140	XHP9	Naming Conventions
Warning	src/applications/files/transform/PhabricatorFileImageTransform.php:145	XHP9	Naming Conventions
Warning	src/applications/files/transform/PhabricatorFileImageTransform.php:153	XHP9	Naming Conventions

Unit

Tests Passed

Build Status

Buildable 901
Build 901: arc lint + arc unit

Event Timeline

• l2dy created this revision.Nov 10 2023, 15:10

Owners added a reviewer: O1: Blessed Committers.Nov 10 2023, 15:10

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptNov 10 2023, 15:10

• l2dy requested review of this revision.Nov 10 2023, 15:10

Harbormaster completed remote builds in B901: Diff 1467.Nov 10 2023, 15:10

Fix lint warnings

Harbormaster completed remote builds in B902: Diff 1468.Nov 10 2023, 15:13

Thanks

Maybe we can mention the upstream commit, and we can amend to set their author information.

I'm trying to understand but I was a bit confused by both changes. I mean, if the first change allows transforms to inherit permissions, why they are still published?

Sorry if this is a stupid question. Need to try :)

Mention upstream commit

Harbormaster completed remote builds in B903: Diff 1469.Nov 10 2023, 15:56

@valerio.bozzolan Upstream removed code that enforces $always_visible when $file->getIsProfileImage() is true in PhabricatorFileQuery.php, but I'm afraid that this change may break other things, so I did not apply it here, but on the other hand kept the description of "Image will be Public".

speck subscribed.Nov 11 2023, 00:03

speck added inline comments.

src/applications/files/transform/PhabricatorFileImageTransform.php
153	The order here is important and we might want to call it out in a comment because it might not be clear when reading the code. The behavior of merging arrays this way is that left-most array’s entries are retained vs right-array. So if a property is defined with getFileProperties() but has a different value from inherit or defaults it will still take precedence.
src/applications/files/transform/PhabricatorFileThumbnailTransform.php
61	What do you think of “Public Profile Image (dimensions)”?

• l2dy added inline comments.Nov 11 2023, 00:27

src/applications/files/transform/PhabricatorFileImageTransform.php
153	The pattern of `+ $defaults` is quite common in Phorge, and I don't see a comment elsewhere.
src/applications/files/transform/PhabricatorFileThumbnailTransform.php
61	I don't think "Public Profile Image" conveys the idea unambiguously. It could be read as an image transform to "Public Profile" dimensions. Also, other options don't contain the word "Image".

• l2dy added inline comments.Nov 11 2023, 00:34

src/applications/files/transform/PhabricatorFileThumbnailTransform.php
61	An image transform that makes an image public irrecoverably is counter-intuitive. I want to capture attention with the extra parentheses and avoid ambiguity as much as possible.