pkp/pkp-lib#13074 Stamp submission last_modified on Author/Citation c… - #13104
pkp/pkp-lib#13074 Stamp submission last_modified on Author/Citation c…#13104monmarzia-archi wants to merge 1 commit into
Conversation
asmecher
left a comment
There was a problem hiding this comment.
Just some early comments -- thanks, @monmarzia-archi!
| */ | ||
| private function dispatchMetadataChanged(int $publicationId): void | ||
| { | ||
| $publication = Repo::publication()->get($publicationId); |
There was a problem hiding this comment.
@monmarzia-archi, there are a couple of downsides to rehydrating the publication from its ID...
- It incurs database activity when that currently isn't necessary for consumers of the event, and
- Generally speaking we risk having several versions of the object in memory and risk having interleaved updates of each clobbering fields.
The first isn't really a problem, since insert/update/delete operations are relatively rare. We'll probably also need some kind of rehydration in order to cascade the update events, i.e. so that an update to an author can cascade to the publication and from there to the submission.
Since the only thing we're doing is updating the "last modified" date, I'd rather we did that as an atomic database operation without re-persisting the whole object. If we do run into a problem with interleaved updates, it makes it clear what part of the code needs to be resolved.
I'm open to counter-proposals on this, but I think it's probably best to change the MetadataChanged event so that it's applicable to changes on any supported entity -- not just submissions but also authors, publications, etc.
So in summary, I think loading the "parent" entity from DB is OK, but
- it'll be necessary to then also emit a new event on that, and
- it's probably better to add an atomic update function on the last modified date in the DAO class.
There was a problem hiding this comment.
Thanks, this gives me a much clearer picture, especially the point about the atomic update. That part is already sketched out on my end (direct DAO-level update, no full entity re-save), pending final testing before I commit it.
On generalizing the event, since you mentioned being open to counter-proposals: I'd suggest introducing new concrete events per entity instead (AuthorMetadataChanged, PublicationMetadataChanged), each with its own listener that resolves the parent and dispatches the next event in the chain, leaving MetadataChanged and its existing listener untouched. One consideration that occurred to me, more as a thought to share than a firm position: since MetadataChanged already has a listener in production, and possibly third-party plugins depending on it too, leaving it untouched means nothing already relying on it today is affected by this change. I'm not sure how much weight that should carry against the benefit of a single generic event, you'd have a much better sense of that than I would, but it seemed worth mentioning given the project's general caution around breaking changes.
My scope so far covers Author, Citation, and Publication, the entities already in 3.5, leaving Funder/DataCitation for later since they're 3.6.0-only. I'd like to know whether this concrete, additive approach covers what you had in mind, or whether there's a reason you'd want MetadataChanged itself made generic beyond this cascade. Either way, is this blocking for this PR, or can we land the current scope (atomic update plus this event chain) and treat a fully generic architecture as a separate follow-up? Part of why I'm asking: I think this fix is foundational to a caching proposal we've been discussing on the forum, whose tag-based invalidation (Section 3.3) depends on entities reliably reporting when their content changed. Getting this right at a contained scope now matters more to me than expanding the redesign, since that's what the caching work will build on.
| * Dispatch MetadataChanged for the submission owning the given publication. | ||
| * Silently no-ops if the publication or submission can't be resolved. | ||
| * | ||
| * @see https://github.com/pkp/pkp-lib/issues/13074 |
There was a problem hiding this comment.
Generally we don't refer to issue numbers from the codebase unless there's an obscure line of code that needs explanation; we can find this from git blame if needed. (Here and elsewhere.) Ideally the code is self-explanatory :)
There was a problem hiding this comment.
Got it! Besides the descriptive comments, I've left a few comment lines referencing #13074 to find them quickly — let me know if I should remove them too.
Draft PR — work in progress, opened for early feedback as requested by @asmecher
Part of #13074 (Track 1 only: Author and Citation entities, targeting stable-3_5_0).
Track 2 (Funder, DataCitation, 3.6.0-only) remains open.Fixes #13074 (only for Author and Citation entities, only on v.3.5)
Summary
Editing an Author or a Citation on a Submission's Publication does not currently update
submissions.last_modifiedon the parent Submission, affecting OAI-PMH harvesting and anyother consumer relying on that field.
This PR introduces a
StampSubmissionModifiedlistener on the existingMetadataChangedevent, and adds dispatch points in
Author\Repository(add, edit, delete, setAuthorsOrder)and
CitationDAO(insert, update, delete/deleteById, deleteByPublicationId, importCitations).Testing done so far
published Submission, confirmed
last_modifiedon the parent Submission updates asexpected.
MetadataChangedListener(search reindexing) is unaffected, sinceboth listeners hook the same event independently.
last_modified, which the OAI-PMH datestamp query already reads from; harvest-levelverification would be useful from reviewers with an OAI-PMH client set up.
Still to do before this leaves draft status
MetadataChangedJobTest/BatchMetadataChangedJobTestremain greenOpen questions for review
Citation on a non-current Publication version? If so, the approach of stamping
the Submission directly may need to be revisited.
AI-assisted development disclosure
Portions of this PR's code analysis and drafting were assisted by an AI tool (Claude).
All technical claims and code have been independently reviewed and verified by me
prior to submission, per PKP's AI contribution policy.