Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deletion hook support #10

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

vedmaka
Copy link
Contributor

@vedmaka vedmaka commented Nov 16, 2023

Support data update trigger upon articles deletion

@vedmaka vedmaka marked this pull request as ready for review November 16, 2023 22:16
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (b2c4db8) to head (a068caa).

Current head a068caa differs from pull request most recent head 6e956de

Please upload reports for the commit 6e956de to get more accurate results.

Files Patch % Lines
src/Hooks.php 0.00% 11 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             master     #10   +/-   ##
========================================
  Coverage      0.00%   0.00%           
  Complexity       28      28           
========================================
  Files             1       3    +2     
  Lines           128     152   +24     
========================================
- Misses          128     152   +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gesinn-it-gea
Copy link
Collaborator

Thanks for your PR. Can you please make sure, that the tests are passing.

@gesinn-it-gea
Copy link
Collaborator

According to https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/docs/technical/hooks/hook.sqlstore.afterdataupdatecomplete.md, the "AfterDataUpdateComplete" should "identify entities that have been added/removed during the update". I'm wondering if on delete, the existing hook wouldn't already work.

vedmaka added 2 commits June 4, 2024 18:55
Support data update trigger upon articles deletion
Changes hook to onPageDelete because there is already no data
by the time SMW::SQLStore::BeforeDeleteSubjectComplete fires.
@labster
Copy link

labster commented Jun 6, 2024

Hi, I manually rebased this change on the master branch, and my colleagues have said that it works well. The PR looks simpler with git diff -b, since there's a lot of whitespace change from an if statement being inserted.

@tosfos
Copy link

tosfos commented Aug 2, 2024

@gesinn-it-gea Can you review this please? Is it still needed?

@vedmaka
Copy link
Contributor Author

vedmaka commented Aug 3, 2024

According to https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/docs/technical/hooks/hook.sqlstore.afterdataupdatecomplete.md, the "AfterDataUpdateComplete" should "identify entities that have been added/removed during the update". I'm wondering if on delete, the existing hook wouldn't already work.

@gesinn-it-gea the ChangeOp param in the hook contains data about properties modified (inc. deleted) and their values, but the SDU hook code does not make use of this to detect if the SDU property is present. The current code uses SMWData getProperties method which will always return an empty array on page deletion. Thus, upon page deletion when the $properties = $newData->getProperties(); returns [] the code thinks that there are no SDU property present and does not perform any updates

Although, you're right that we don't need a separate hook to handle the deletion and it's possible to rewrite the existing hook to make use of the getOrderedDiffByTable of the ChangeOp to detect the presence of the SDU property on page deletion and it's former value

I'll try to follow up with a patch rewrite

)

Initializing the git submodule on some systems ends with the error:
    fatal: Could not read from remote repository
We often initialize/update the submodules recursively, this interferes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants