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

Last modified timestamp not updated when updating metadata using the Doctrine adapter #530

Open
fangel opened this Issue Mar 28, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@fangel
Member

fangel commented Mar 28, 2017

I stumbled upon a discrepancy in the Doctrine database-adapter, namely that it the updateMetadata and deleteMetadata methods doesn't update the modified-timestamp of the image.

This is, however, the case for the Mongo and MongoDB adapters.

As an example, look at theupdateMetadata-methods for the Mongo and MongoDB adapters: https://github.com/imbo/imbo/blob/develop/src/Database/Mongo.php#L201 + https://github.com/imbo/imbo/blob/develop/src/Database/MongoDB.php#L185 where the updated-property gets updated to time()

If we compare this to the updateMetadata-method of the Doctrine adapter, we can see that such an update does not occur: https://github.com/imbo/imbo/blob/develop/src/Database/Doctrine.php#L158-L184


In the IRC-channel, it was discussed changing the interface for the database-class to include a method to explicitly update the last-modified timestamp, and then have it be the responsibility of the resource-handlers to update the timestamp when necessary.

My proposal would be to bug-fix the Doctrine adapter for Imbo 2.x, and then modify the Imbo\Database\DatabaseInterface-interface and the adapters for Imbo 3.x (since I would classify modifying that interface as breaking BC, as someone could have made their own database-adapter based on the interface.

If you agree, I'll create two PRs with the bug-fix + update.

-Morten.

@christeredvartsen

This comment has been minimized.

Member

christeredvartsen commented Jul 5, 2017

Sorry for not replying to this sooner.

I think this sounds like a nice addition. Do you have anything to add @matslindh?

@matslindh

This comment has been minimized.

Contributor

matslindh commented Jul 5, 2017

👍 - I want the logic of "updating metadata updates the timestamp of the image" to part of the general flow instead of up to the adapter, so the different strategies for Imbo2 and Imbo3 seems sane.

@fangel

This comment has been minimized.

Member

fangel commented May 15, 2018

I finally got around to creating the "easy" fix for the 2.x-branch (see #597).

Is the preferred approach for 3.x still to create a new method on the DatabaseInterface with a signature akin to public function markAsUpdated($user, $imageIdentifier); that can then explicitly be called from the put and post-handlers on the Metadata resource?

If so, I'll create a PR for that against the 3.x branch. What name should this new method have?

@matslindh

This comment has been minimized.

Contributor

matslindh commented May 15, 2018

I'd prefer if we kept the notion of time in the method name, so that it's clear that we're updating the timestamp of when the image was updated - and not just saying that it has been updated. How about something like setUpdatedTimeNow($user, $imageIdentifier) - this will allow us to introduce a setUpdatedTime later if necessary.

@fangel

This comment has been minimized.

Member

fangel commented May 15, 2018

How about setUpdatedTime($user, $imageIdentifier, $time=time())? Then we wouldn't have to add a second method (and thus break the interface) later on...

@matslindh

This comment has been minimized.

Contributor

matslindh commented May 15, 2018

PHP doesn't allow function calls as default parameters - so we'd have to do $time = null and make all the adapters assume that this means the current time. In that case I'd prefer if we implemented two methods, one for setUpdatedTime and one for setUpdatedTimeNow (or setUpdatedNow which I might prefer) - since we don't have an abstract class here (but this could be a case for introducing one), we can't automagically provide an implementation for setUpdatedTimeNow() as setUpdateTime(..., $timestring).

@fangel

This comment has been minimized.

Member

fangel commented May 15, 2018

I'll introduce setUpdatedTime and setUpdatedNow in the interface - but I might skip adding in an abstract class if that's okay for now.

fangel added a commit to tv2/imbo that referenced this issue May 17, 2018

Added methods to update last-modified timestamp to DB interface
This moves the responsibility for updating the last-modified timestamp away from the update-metadata functionality,
and makes it into an explicit action performed by the database-operations event-listener

This is in reference to imbo#530, where the behaviour between the Doctrine and Mongo implementation were different.
@fangel

This comment has been minimized.

Member

fangel commented May 17, 2018

I've created a PR (#598) now - I changed the method names to setLastModifiedNow and setLastModifiedTime because there was a already existing method called getLastModified so I figured I'd stick to that naming.

fangel added a commit to tv2/imbo that referenced this issue May 23, 2018

Added methods to update last-modified timestamp to DB interface
This moves the responsibility for updating the last-modified timestamp away from the update-metadata functionality,
and makes it into an explicit action performed by the database-operations event-listener

This is in reference to imbo#530, where the behaviour between the Doctrine and Mongo implementation were different.

christeredvartsen added a commit that referenced this issue May 24, 2018

Added methods to update last-modified timestamp to DB interface (#598)
This moves the responsibility for updating the last-modified timestamp away from the update-metadata functionality,
and makes it into an explicit action performed by the database-operations event-listener

This is in reference to #530, where the behaviour between the Doctrine and Mongo implementation were different.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment