wordpress-mobile / WordPress-Android

WordPress for Android
http://android.wordpress.org
GNU General Public License v2.0
2.99k stars 1.32k forks source link

Local media ids can collide with remote ids #11239

Open mkevins opened 4 years ago

mkevins commented 4 years ago

Description

The local media ids used for not-yet-uploaded media files can collide with remote ids of items in the WordPress Media Library. In most cases, this should not cause a problem, however, situations may exist where the id conflict poses an issue. In particular, we currently deduplicate media items within the gallery block keyed on id, so this could affect user experience for a user uploading new items with local ids that match the ids of items already added.

This issue was previously noted here: https://github.com/wordpress-mobile/gutenberg-mobile/issues/1610

There is a potential solution to this issue here: https://github.com/wordpress-mobile/WordPress-Android/pull/11125, but this may require further investigation to explore alternative solution(s) (another possible solution is mentioned in the PR description).

Update:

This issue has been confirmed to be related to and a possible root cause of https://github.com/wordpress-mobile/gutenberg-mobile/issues/1853.

:1st_place_medal: Thanks to @cameronvoell and @mchowning:

Steps to reproduce the behavior

Expected:

Actual:

mchowning commented 4 years ago

I looked into this a little bit while investigating https://github.com/wordpress-mobile/gutenberg-mobile/issues/1853.

It seems that the problem comes up because, within Gutenberg, we don't differentiate between the local id and the remote id when we set an image block's id attribute (whichever one we have, we set to the block's id attribute). Then, within WPAndroid, when an upload completes, we do a regex search for any blocks that have an id matching the local id of the uploaded image, and we then replace the id with the remote id of the just uploaded image (which may have not actually been the local id of the uploaded image, it could have been the remote id of a different image, see https://github.com/wordpress-mobile/gutenberg-mobile/issues/1896).

Your idea of negating the local id for gutenberg seems like it may be a promising avenue. We would just need to also include that that negation logic in any other place we use an image id from gutenberg in the native code (such as discussed above when we update an image block after it's upload is complete)

mchowning commented 4 years ago

When looking into this issue, we may also want to keep https://github.com/wordpress-mobile/gutenberg-mobile/issues/1526 in mind. Since it (possibility of publishing the local file url in the block even when an image has been successfully uploaded) is also a result of our current local id/url handling imo.

hypest commented 4 years ago

This issue has been confirmed to be related to and a possible root cause of wordpress-mobile/gutenberg-mobile#1853

https://github.com/wordpress-mobile/gutenberg-mobile/issues/1853 is now closed. Is this ticket still valid or not?

mkevins commented 4 years ago

The underlying issue is still possible, AFAIK, but the symptoms exhibited in wordpress-mobile/gutenberg-mobile#1853 seem to be resolved. I'm not sure how common / likely the collisions described in this issue are to occur, but it could result in some consistency issues with the React rendering of galleries, in theory, since we use id as a key. Also, there is a potential edge case from the collisions that the wrong id would be replaced in the upload completion processing flow (when upload completes while the editor is closed).

mchowning commented 3 years ago

User report of a similar sounding issue:

User on Android mobile app and in a post with a lot of images in image blocks, when they tried to replace one image, it was replacing others in the post with that same image.

3580046-zd-woothemes

mchowning commented 2 years ago

Related to https://github.com/wordpress-mobile/gutenberg-mobile/issues/1526