Details

src/applications/phriction/controller/PhrictionEditController.php
124	If I change this to phutil_json_decode, the if statements below won't work correctly anymore as $content_metadata becomes an array instead of a stdClass

valerio.bozzolan added inline comments.Jun 30 2024, 13:17

src/applications/phriction/controller/PhrictionEditController.php
124	Yep sorry, it becomes `$content_metadata['attachedFilePHIDs']` 👍

Updates after comments

Harbormaster completed remote builds in B1384: Diff 2172.Jun 30 2024, 13:27

Update after comments

Harbormaster completed remote builds in B1385: Diff 2173.Jun 30 2024, 13:29

mturdus marked 2 inline comments as done.Jun 30 2024, 13:30

Fix file attachment in Conpherence

Summary:
when you drag and drop a file in a Conpherence chat, the file is only visible by the author.

Refs T15106

Test Plan:

open the Conpherence chat and join a room
drag and drop file in it
send message
verify file if file can be seen/downloaded by other user

Harbormaster completed remote builds in B1388: Diff 2176.Jun 30 2024, 17:07

(Do you need a little help to split the changes in two diffs?)

You may like also this:

$attached_phids = idx($metadata, 'attachedFilePHIDs', array());

Found here:

https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php$2297

So just do isset($attached_phids) && is_array(...) etc.

Well, to be honest, feel free to generalize the description and commit message to include both changes, since they are the very same thing

Updates after comments

Harbormaster completed remote builds in B1390: Diff 2178.Jun 30 2024, 17:44

mturdus retitled this revision from Fix file attachment in Phriction to Fix file attachment in Phriction and Conpherence.Jun 30 2024, 17:46

mturdus edited the summary of this revision. (Show Details)

mturdus edited the test plan for this revision. (Show Details)

In D25705#19485, @valerio.bozzolan wrote:

(Do you need a little help to split the changes in two diffs?)

yes :-)

I ran arc diff and I thought it would create a 2nd diff revision.

OK try this to restore your desired situation (so, two diffs)

git stash save

git checkout -b just_phriction
git reset --hard origin/master
arc patch --nobranch --diff 2173
arc diff --update D25705

git checkout -b just_conpherence
git reset --hard origin/master
arc patch --nobranch --diff 2178
arc diff --create

Then you have this benefit that you can repeat as many times you want:

git checkout just_phriction
LOL DO CHANGES
arc diff

git checkout just_conpherence
LOL DO CHANGES
arc diff

Or, get fresh master again:

git checkout master

Ideally, the default assumes that people do not touch master but work in branches, each branch a different feature

dedicating Conpherence its own diff as mturdus requested

Harbormaster completed remote builds in B1392: Diff 2180.Jul 1 2024, 09:24

we can remove an undeclared variable, and add a nonempty-str check

Harbormaster completed remote builds in B1393: Diff 2181.Jul 1 2024, 09:29

Thanks my friend, this works perfectly

This revision is now accepted and ready to land.Jul 1 2024, 09:31

valerio.bozzolan mentioned this in D25709: Fix missing file attachment in Conpherence.Jul 1 2024, 11:58

valerio.bozzolan mentioned this in D25710: [STUB] Fix missing file attachment in Conpherence.Jul 1 2024, 12:07

We can avoid phutil_nonempty_string() since PHP pitfalls "Check for non-empty strings" are under control here.

https://we.phorge.it/book/flavor/article/php_pitfalls/

Aaaaand... this was my last contribution here. Thanks again.

Harbormaster completed remote builds in B1405: Diff 2194.Jul 1 2024, 13:37

In D25705#19508, @valerio.bozzolan wrote:
OK try this to restore your desired situation (so, two diffs)
git stash save

git checkout -b just_phriction
git reset --hard origin/master
arc patch --nobranch --diff 2173
arc diff --update D25705

git checkout -b just_conpherence
git reset --hard origin/master
arc patch --nobranch --diff 2178
arc diff --create
Then you have this benefit that you can repeat as many times you want:
git checkout just_phriction
LOL DO CHANGES
arc diff

git checkout just_conpherence
LOL DO CHANGES
arc diff
Or, get fresh master again:
git checkout master
Ideally, the default assumes that people do not touch master but work in branches, each branch a different feature

Thanks I will remember this for the future :-)

But since you have split these 2 up, I'm a bit confused now how I can land only the Phriction part.
I'm not that familiar with arc as you already might have noticed.

This is my current git status and show HEAD state:

gj@blackbird:/var/www/htdocs/phorge/phorge$ git status
On branch master
Your branch is ahead of 'origin/master' by 1 commit.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean
gj@blackbird:/var/www/htdocs/phorge/phorge$ git show --name-only HEAD
commit 8e8372e2a79a226bd874858488c6738c784e87ee (HEAD -> master)
Author: Merula Turdus <merula.turdus@outlook.be>
Date:   Sat Jun 29 20:08:40 2024 +0200

    Fix file attachment in Phriction
    
    Summary:
    when you drag and drop a file in the Phriction editor and then save the
    corresponding wiki document, the file is only visible by the author.
    
    Refs T15106
    
    Test Plan:
    1) open the Phriction editor to edit an existing or create a new wiki document
    2) drag and drop file in it
    3) save wiki document
    4) verify file is attached to wiki document (other user can see file in wiki page)
    
    Reviewers: O1 Blessed Committers!
    
    Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
    
    Maniphest Tasks: T15106
    
    Differential Revision: https://we.phorge.it/D25705

src/applications/conpherence/controller/ConpherenceUpdateController.php
src/applications/phriction/controller/PhrictionEditController.php

Are these the correct steps:

git reset HEAD~1
git restore src/applications/conpherence/controller/ConpherenceUpdateController.php
arc diff --update D25705
arc land

If you would like to download the current version of D25705 to maybe land that, just:

arc patch D25705
arc land

Or, to just download the current version of D25705 to eventually add some local extra changes, just:

arc patch D25705

DO CHANGES
arc diff

DO CHANGES
arc diff

DO CHANGES
arc diff

Closed by commit rP118f989366a0: Fix missing file attachment in Phriction. · Explain WhyJul 1 2024, 16:41

This revision was automatically updated to reflect the committed changes.

mturdus added a commit: rP118f989366a0: Fix missing file attachment in Phriction.

Thanks

valerio.bozzolan mentioned this in Next Up.Jul 1 2024, 16:58

valerio.bozzolan awarded a token.Jul 1 2024, 17:46

mturdus mentioned this in rP136091ecd097: Fix missing file attachment in Conpherence.Jul 5 2024, 16:20

valerio.bozzolan mentioned this in 2024 week 35.Aug 28 2024, 10:19

Fix missing file attachment in Phriction
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 2173

src/applications/phriction/controller/PhrictionEditController.php

Fix missing file attachment in PhrictionClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 2173

src/applications/phriction/controller/PhrictionEditController.php

Fix missing file attachment in Phriction
ClosedPublic
Actions

Revision Contents
Changeset List