Repository navigation
Fix PHP deprecation warnings (null array offsets, output handler output, object cache key) - #1340
Conversation
…nt parts PHP 8.5 deprecates using null as an array offset. Relationship::get_relationship() and Delivery::get_sized() can be called with a null ID (e.g. when the cloudinary_contextualized_post_id filter returns null), and Component::get_part() is called with a null part by the Opt_Level component. Cast the IDs to int (the Relationship constructor already does so) and skip the build parts lookup for a null part.
PHP 8.4 deprecates producing output from within a user output handler
("ob_end_flush(): Producing output from user output handler
Cloudinary\String_Replace::replace_strings is deprecated").
The double ob_start() is not the cause on its own: the deprecation fires when
an error is displayed while the handler runs (e.g. notices raised by
cloudinary_string_replace callbacks, with display_errors on), since that
display is output produced inside the handler and is discarded anyway.
Route the buffers through a dedicated output_handler() that disables
display_errors while replacing strings and restores it afterwards. Errors are
still logged. Both buffers are kept to preserve the early-flush workaround.
Delivery::find_attachment_size_urls() reused $key inside the results loop for each attachment URL, so wp_cache_add() stored the results under the last attachment's URL instead of md5( $sql ). When the last wp_get_attachment_url() call failed, the key was false, triggering "WP_Object_Cache::add was called incorrectly. Cache key must be an integer or a non-empty string, boolean given". Use a separate variable for the attachment URL, and skip attachments without a URL rather than mapping them under a false key.
… version Media::get_cloudinary_version() passed the parent attachment's version meta straight to preg_replace(). When the parent has no version meta, get_post_meta() returns its null default, triggering "preg_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated". Default the meta to an empty string, which still resolves to version 1.
The cloudinary_contextualized_post_id filter can return null, e.g. WPML's
wpml_object_id for an attachment without a translation in the current
language. Delivery::find_attachment_size_urls() then carried on with a null ID,
calling create_delivery( null ), which ends in wp_cache_delete( null )
("WP_Object_Cache::delete was called incorrectly. Cache key must be an integer
or a non-empty string, NULL given"), and add_to_sync( null ).
Skip the result when the contextualized ID is empty.
| } | ||
| } | ||
| } | ||
| wp_cache_add( $key, $cached ); |
There was a problem hiding this comment.
Warning: This fix turns on a cache that was never used before. On develop, $key changed inside the loop, so wp_cache_get( md5( $sql ) ) always missed. Every request ran create_delivery() / add_to_sync() again. With a persistent object cache:
- The entry has no group and no expiration. If the delivery or the sync fails, the URLs stay unknown until the cache is cleared.
- The key uses only the SQL, but
cloudinary_contextualized_post_id(WPMLwpml_object_id) gives a different result for each language. The first language fills the cache, and the other languages skipcreate_delivery()/add_to_sync()for their translated IDs.
Suggestion: add an expiration (for example, HOUR_IN_SECONDS) and add the media context or language to the key.
There was a problem hiding this comment.
@utkarshcloudinary Good catch, thanks. You're right that this turned on a cache that had never actually worked.
Rather than adding an expiry and putting the language in the key, I've limited it to the current request with a static array, like get_sized() / get_relationship().
Its real value is avoiding a repeat lookup within one request, since the double output buffer can run it twice.
Across requests the behaviour is now the same as develop, so failed deliveries or syncs get retried and each language resolves its own IDs. An expiry would still have delayed retries by up to an hour.
This feels like a safer change. Let me know your thoughts!
| * | ||
| * @return string | ||
| */ | ||
| public function output_handler( $buffer, $phase ) { |
There was a problem hiding this comment.
Nit: This handles errors that are displayed, but a cloudinary_string_replace callback that uses echo still produces output and still triggers the PHP 8.4 deprecation. One line in the docblock about this limit is enough.
There was a problem hiding this comment.
Agreed, I've added a line to the docblock noting that callbacks which echo directly aren't covered.
| 'name' => $part, | ||
| ); | ||
| if ( isset( $this->build_parts[ $part ] ) ) { | ||
| if ( null !== $part && isset( $this->build_parts[ $part ] ) ) { |
There was a problem hiding this comment.
Nit: The only caller that sends null is php/ui/component/class-opt-level.php:122 (get_part( null )). A fix at that call site is more direct. Optional.
Fixes recurring PHP deprecation/warning logs. There's one commit per fix.
Approach
Relationship::get_relationship(),Delivery::get_sized(),Component::get_part()): cast the IDs to int.Opt_Levelnow callsget_part( '' )instead ofget_part( null ).String_Replaceoutput handler: the doubleob_start()isn't the cause. Errors displayed during the handler count as output. The buffers now go throughoutput_handler(), which turns offdisplay_errorswhile it runs. Errors are still logged.WP_Object_Cache::addcalled with a boolean key (Delivery::find_attachment_size_urls()): the loop no longer overwrites$key, so results are cached undermd5( $sql )as intended. The cache lasts for the current request only, so failed deliveries are retried and each language resolves its own IDs.preg_replace()given a null subject (Media::get_cloudinary_version()): the parent's version lookup now defaults to''.WP_Object_Cache::deletecalled with a NULL key (Delivery::find_attachment_size_urls()): results are skipped when the contextualized ID is empty (WPML returnsnullfor an untranslated attachment). This also stops thecreate_delivery( null )andadd_to_sync( null )calls.QA notes
WP_DEBUGandWP_DEBUG_LOGon, browse the front end and the media library.debug.logshould contain none of the messages above.nullfor an untranslated attachment:add_filter( 'cloudinary_contextualized_post_id', '__return_null', 100 );.<img src="<uploads URL>/repro/unknown.png">inwp_footer.add_post_meta( <any attachment ID>, '_wp_attached_file', 'repro/unknown.png' ).developlogs all five messages, and this branch logs none. phpstan and PHPUnit pass.