Skip to content

fix(node): emit hooks on Node::copy() - #52996

Merged
mejo- merged 2 commits into
masterfrom
fix/emit_hooks_on_copy
May 28, 2025
Merged

fix(node): emit hooks on Node::copy()#52996
mejo- merged 2 commits into
masterfrom
fix/emit_hooks_on_copy

Conversation

@mejo-

@mejo-mejo- commented May 20, 2025•
edited
Loading

Copy link
Copy Markdown
Member

When calling Files\Node\Node::copy(), Files\View::copy() gets called, but Files\View::fakeRoot is empty so the hooks are not emitted if no path is given to Files\View::shouldEmitHooks().

This results in node-related events like NodeCopiedEvent not being fired when copying files via Files\Node\Node::copy().

Files\View::shouldEmitHooks() is given a path as parameter in almost all places except when called from the copy() function. This commit changes it and passes the copy target path.

Fixes: nextcloud/collectives#1756

Checklist

@mejo-mejo- self-assigned this May 20, 2025
@mejo-
mejo- requested a review from a team as a code ownerMay 20, 2025 15:16
@mejo-mejo- added the bug label May 20, 2025
@mejo-
mejo- requested review from skjnldsv and removed request for a teamMay 20, 2025 15:16
@github-project-automationgithub-project-automationBot moved this to 🧭 Planning evaluation (don't pick) in 📝 Productivity teamMay 20, 2025
@mejo-mejo- moved this from 🧭 Planning evaluation (don't pick) to 👀 In review in 📝 Productivity teamMay 20, 2025
@come-nc

come-nc commented May 20, 2025

Copy link
Copy Markdown
Contributor

@icewind1991
Hello, I debugged this with @mejo- .
We came to the conclusion that the shouldEmitHooks call was missing the path. Because most other calls give a path to it.

When copying through dav, fakeRoot is set to the files folder, and that matches the default root, and thanks to that the hook is emitted.
But when copying from somewhere else (collectives in this case) through the node API, fakeRoot is empty.

Do you thing this is the right fix? Do you think we should give the path of source, target, or both?

skjnldsv
skjnldsv approved these changes May 21, 2025
View reviewed changes

@skjnldsvskjnldsv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems logical

icewind1991
icewind1991 approved these changes May 26, 2025
View reviewed changes

@icewind1991icewind1991 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

@mejo-
mejo-force-pushed the fix/emit_hooks_on_copy branch from b6c76a3 to bf67d8fCompareMay 26, 2025 16:30
@mejo-

mejo- commented May 26, 2025

Copy link
Copy Markdown
MemberAuthor

/backport to stable31

@mejo-

mejo- commented May 26, 2025

Copy link
Copy Markdown
MemberAuthor

/backport to stable30

@mejo-mejo- mentioned this pull request May 27, 2025
Closed
@mejo-
mejo-force-pushed the fix/emit_hooks_on_copy branch 2 times, most recently from e427f00 to 1dd0ec8CompareMay 27, 2025 08:39
artonge
artonge reviewed May 27, 2025
View reviewed changes

@artongeartonge left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, maybe just look into what the handleMove method is doing, as it seems to be doing something very similar.

Comment threadapps/files/lib/Listener/SyncLivePhotosListener.php Outdated
Comment threadapps/files/lib/Listener/SyncLivePhotosListener.php Outdated
@mejo-
mejo-force-pushed the fix/emit_hooks_on_copy branch from 1dd0ec8 to f77ae68CompareMay 27, 2025 09:23
@mejo-
mejo- requested a review from artongeMay 27, 2025 09:24
mejo- added 2 commits May 27, 2025 12:14
@mejo-
fix(node): emit hooks on Node::copy()
c952570
When calling `Files\Node\Node::copy()`, `Files\View::copy()` gets called,
but `Files\View::fakeRoot` is empty so the hooks are not emitted if no
path is given to `Files\View::shouldEmitHooks()`.
This results in node-related events like `NodeCopiedEvent` not being
fired when copying files via `Files\Node\Node::copy()`.
`Files\View::shouldEmitHooks()` is given a path as parameter in almost
all places except when called from the `copy()` function. This commit
changes it and passes the copy target path.
Fixes: nextcloud/collectives#1756
Signed-off-by: Jonas <jonas@freesources.org>
@mejo-
fix(SyncLivePhotosListener): Don't handle copy event emitted from us
e5b4ae4
Running $peerFile->copy() causes a second BeforeNodeCopiedEvent now,
which we don't want to handle.
Signed-off-by: Jonas <jonas@freesources.org>
@mejo-
mejo-force-pushed the fix/emit_hooks_on_copy branch from f77ae68 to e5b4ae4CompareMay 27, 2025 10:14
@mejo-
mejo- merged commit f833e2d into masterMay 28, 2025
@mejo-
mejo- deleted the fix/emit_hooks_on_copy branch May 28, 2025 11:49
@github-project-automationgithub-project-automationBot moved this from 👀 In review to ☑️ Done in 📝 Productivity teamMay 28, 2025
This was referenced May 28, 2025
Merged
Merged
@skjnldsvskjnldsv mentioned this pull request Aug 19, 2025
Merged
@salmart-devsalmart-dev mentioned this pull request Mar 2, 2026
Merged
7 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@icewind1991icewind1991icewind1991 approved these changes
@artongeartongeartonge approved these changes
@skjnldsvskjnldsvskjnldsv approved these changes
@juliusknorrjuliusknorrAwaiting requested review from juliusknorr
@come-nccome-ncAwaiting requested review from come-nc

Assignees

@mejo-mejo-

Labels

3. to reviewWaiting for reviewsbugfeature: filesystem

Projects

📝 Productivity team
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Attachments get lost when copying a page

5 participants

@mejo-@come-nc@icewind1991@artonge@skjnldsv