Upgrade node & composer packages - #1263
Merged
Merged
Conversation
Not for merge -- diagnosing the bulk_sync_started asset_count and deactivation_modal_viewed failures via CI logs.
Analytics.track() passed a full absolute URL via apiFetch's `path` option, which builds a URL relative to the REST API root instead of using one verbatim -- the resulting request 404'd, so the client-side `deactivation_modal_viewed` event never reached the server. Use `url` instead, which apiFetch passes through unmodified. trackReliable()'s sendBeacon() path was unaffected since it builds its own URL. Also fixes the wp-cli debug helper in sync-analytics.spec.js's temporary instrumentation to use wpEvalFile (plain `wp eval` can't see get_plugin_instance()), so it can gather evidence for the still-open bulk_sync_started asset_count failure on the next CI run.
fakeCloudinaryConnected() bypasses Connect::verify_connection() (to avoid live API calls), which is what normally sets auto_sync to 'off' on a real connect -- so it stayed at its 'on' default. Visiting the Cloudinary admin page then kicks off a background autosync thread that can claim the test's freshly-inserted attachment (setting its `queued` meta) before the manual /sync REST call's build_queue() gets to it, leaving nothing for the manual queue to find. Turn auto_sync off alongside the other preconditions so this test exercises the manual sync path in isolation, matching its own `trigger: manual` assertion. Confirmed via temporary CI instrumentation (removed here) that image_delivery persisted correctly and matching attachments existed at read time -- the option build_queue() writes was simply never created because the query it ran during the manual REST request found nothing. Also removes the now-unneeded diagnostic prints from the previous commit.
Not for merge -- the auto_sync=off fix didn't resolve asset_count still being 0; checking what build_queue()'s query actually sees at REST-call time.
rest_start_sync() read the just-built queue's total back from the _cloudinary_sync_queue option *after* calling start_queue(), which already kicks off the background sync threads for it. A thread that finishes (or errors out) fast enough calls stop_queue(), which deletes that option, before the original request gets a chance to read it back -- so the analytics event it fires can report asset_count: 0 even though build_queue() found and queued assets moments earlier in the same request. Have Sync_Queue capture the total synchronously as build_queue() computes it (get_last_built_total()), and read that instead of re-reading the racy shared option. Confirmed via temporary CI instrumentation (removed here): the auto_sync=off fix from the previous commit was necessary but not sufficient -- with it in place, build_queue() still found the test's attachment and started processing it (visible via a leaked "Asset in sync loop." meta on prior attempts' attachments), just not fast enough to beat this race.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Approach
QA notes