Optimized the upgrade script #1
Conversation
juho-jaakkola
commented
Jan 7, 2014
juho-jaakkola
commented
- We're not triggering event handlers so there's no need to deregister them
- Removed DB queries from foreach loops
- Uses correct coding style
- We're not triggering event handlers so there's no need to deregister them - Removed DB queries from foreach loops - Uses correct coding style
|
Do not merge yet. I made the PR just that it's easier to discuss it. |
| $query = "UPDATE {$db_prefix}river | ||
| SET view = 'river/object/comment/image' | ||
| WHERE id IN ($river_entry_ids)"; | ||
| update_data($query); |
There was a problem hiding this comment.
I first looped through all the river entries and collected their ID's. Thanks to this it's possible to do only one DB query instead of one query per each found river entry.
|
I haven't tested this yet because I do not currently have any test data. Do you have an environment where you could test it? |
| WHERE id = {$river_entry->id} | ||
| "; | ||
| update_data($query); | ||
| // fix target_guid and access_id for river entries that do not yet point to the album |
There was a problem hiding this comment.
This last part is a bit weird. Was it possible to comment image batches instead of albums in the 1.8 version? Or why do we need to change the river target from image_batch into album?
There was a problem hiding this comment.
In Tidypics 1.8.0RC (last "official" release) the river entry created on uploading new images had the tidypics_batch entity as object_entity. I kept this unchanged. The problem was when commenting directly on the activity page such an entry the comments did only show up in the activity river attached to the upload entry but not on the album page or any image within this album.
So, I changed this in the following way already in my forked 1.8 version of Tidypics:
- the original comment (annotation) to the tidypics_batch entity is still created (and will still show up only on the activity page). But there is no new separate river comment entry created.
- in addition there's a second comment annotation created (with the same content) that is attached to the album the tidypics_batch belongs to. This comment will then also show up on the album page. For this comment annotation a new river entry was created (with album as object entity).
I think this way the commenting on Tidypics entities (with the three subtypes album, image and tidypics_batch) the behaviour is more on the line of other plugins with only one subtype (where comments made in the river are also displayed on the full view page of these entities).
What I did not in the Elgg 1.8 tree of my Tidypics fork is updating the already existing tidypic_batch river comments. But I thought the upgrade of an existing site to Elgg 1.9 (which made it necessary to update the river entries for images and albums anyway after the Elgg core upgrade script that converts the comment annotations to entities) would be a good opportunity for upgrading the old river entries of tidypics_batches to match the newly created entries.
|
Thanks for PR. I'll study the changes and wait with the merge - as I really would like to test it first, too. :-) I don't have a suitable test environment available but I will simply set up a new Elgg 1.8 site, upload some images and comment on them. This might not be test case to judge the run time of the upgrade script or check for memory issues as I don't think I'll create 1000s of comments but I hope that it'll be enough to check the database entries after the upgrade of Elgg to 1.9 and an upgrade of Tidypics. I'm afraid I might only be able to do the testing next weekend though. Btw. I originally wanted to require a minimum Elgg_version (as opposed or in additon to a minimum Elgg_release) to make sure that the conversion of comment annotations to entities has already been finished. Otherwise the conversion done by Elgg core would change the view again after the Tidypics upgrade script has been executed. Unfortunately, it's no longer possible to require an Elgg_version in the manifest file but only an Elgg_release. Is there another possible way to stop site admins from activating Tidypics in the first place if the core upgrade has not been done yet? and best would be to check if not only the first 50 annotations are converted by the upgrade script but also all the others. |
|
I re-worked the upgrade script for release beta13. I kept the optimizations you suggested for the river "view" updates (single update_data instead of foreach loop) - had to make some changes though to get it bug-free (get_subtype_id() has two arguments ;-)). For the other parts of the update I don't think a single update_data is possible instead of the foreach loops because of the dependency on the item's specific data. I guess the script will be time-consuming but at least I don't see any better way. The upgrade now does all the necessary changes to get the stuff from older Tidypics versions to be the same as for future content (basically moving any comments and likes made on tidypics_batches either to album or image entities). The Elgg 1.8 version of Tidypics 1.8.1beta13 now includes a corresponding upgrade script. So any user of the Tidypics plugin is free to upgrade the entries already now on Elgg 1.8 or at the latest when upgrading to Elgg 1.9. The new upgrade script has a new filename (removed the old one). Should I close this issue for now and in case there's more stuff to discuss we could open a new issue? |
|
Sure, free feel to close it if you've already added the contents to Tidypics. It's usually best to open a new PR/discussion/etc for each new issue/topic/etc. It's easier to handle problems in small pieces. |
