I think I've found the issues with the database and S3 not staying in sync, as referenced here: https://www.drupal.org/project/advagg/issues/2972593

I have two patches which I will post in separate threads. This one adds some code to the advagg_rename function.

Instead of using the PHP rename function, I'm using the public rename function available in the S3fsStreamWrapper. The benefit of using this function is that it calls the waitUntilFileExists function, ensuring the process is not officially completed until the file actually exists in S3.

Another point to mention is advagg_rename uses drupal_realpath. I'm skipping this for s3fs, because as it says on its page:

The use of drupal_realpath() is discouraged, because it does not work for remote URIs. Except in rare cases, URIs should not be manually resolved.

https://api.drupal.org/api/drupal/includes%21file.inc/function/drupal_re...

Please review and let me know if you have questions, thanks.

CommentFileSizeAuthor
#12 advagg-2973383-12-try-cascade-rename.patch1.22 KBmikeytown2
#11 advagg-2973383-11-try-cascade-rename.patch1.46 KBmikeytown2
#10 advagg-2973383-10-try-cascade-rename.patch1.27 KBmikeytown2
#7 advagg-cascading_rename_options-2973383-7.patch1.95 KBsgdev
advagg-use_s3_rename_function-1.patch1.19 KBsgdev

Comments

ron_s created an issue. See original summary.

sgdev’s picture

sgdev commented

I noticed in the code it is mentioned about wanting to keep the rename command local and atomic.

Also, I've done more research and I don't think this is the cause of the problems I am seeing (I'm going to detail my findings on another post).

So if you'd prefer to close this issue and not add the patch, I'm totally ok with that. Let me know.

  • mikeytown2 committed 94b8166 on 7.x-2.x authored by ron_s 
    Issue #2973383 by ron_s: Use public rename function included with s3fs
mikeytown2’s picture

mikeytown2 commented
Status: Needs review » Fixed

Using the s3 rename is ok here as it's a fairly unique situation.

mikeytown2’s picture

mikeytown2 commented
Status: Fixed » Active

Thinking about this and should it cascade through and try rename then file_unmanaged_move and then the rename method if it exists? That way it'd work for other stream wrappers.

sgdev’s picture

sgdev commented

That sounds like a good idea. Let me create a new patch for review.

sgdev’s picture

sgdev commented
Title: Use public rename function included with s3fs » Cascading rename options
Status: Active » Needs review
StatusFileSize
new advagg-cascading_rename_options-2973383-7.patch1.95 KB

See the attached patch. I moved file_unmanaged_move ahead of @rename. Let me know if you are ok with that.

Also moved the creation of $real_source and $real_destination inside the conditional. No need to define them until they are needed.

Status: Needs review » Needs work

The last submitted patch, 7: advagg-cascading_rename_options-2973383-7.patch, failed testing. View results

sgdev’s picture

sgdev commented
Status: Needs work » Needs review

Not sure the Call to a member function rename() on boolean is a true error?

I get this in my IDE too, and see it in other modules like Transliteration that call $wrapper->rename().

mikeytown2’s picture

mikeytown2 commented
StatusFileSize
new advagg-2973383-10-try-cascade-rename.patch1.27 KB
mikeytown2’s picture

mikeytown2 commented
StatusFileSize
new advagg-2973383-11-try-cascade-rename.patch1.46 KB
mikeytown2’s picture

mikeytown2 commented
StatusFileSize
new advagg-2973383-12-try-cascade-rename.patch1.22 KB
sgdev’s picture

sgdev commented
Status: Needs review » Reviewed & tested by the community

Have been using #12 for the past 18 months on live sites with no issues... setting as RTBC?

  • mikeytown2 committed 62d6c05 on 7.x-2.x 
    Issue #2973383 by mikeytown2, ron_s: Cascading rename options
mikeytown2’s picture

mikeytown2 commented
Status: Reviewed & tested by the community » Fixed

  • mikeytown2 committed c58daab on 7.x-2.x 
    Issue #2973383 by mikeytown2, ron_s: Cascading rename options
sgdev’s picture

sgdev commented

@mikeytown2, thanks for committing #12.

It seems like the code committed in #14 is something different... new code I've never seen before for advagg_js_compress_do_it.

Just wanted to make sure this wasn't committed by accident?
https://git.drupalcode.org/project/advagg/commit/62d6c05

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented

I must mark it fixed, for lack of interaction.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.