Skip to content

Conversation

@felixarntz
Copy link
Member

@felixarntz felixarntz commented Mar 11, 2024

Summary

See #1042 (comment): Given that PR contains changes to the object-cache.php drop-in which require updating it in order for the underlying Server-Timing logic to still be loaded correctly, the logic to place that file needs to support updating an older version of the file to its latest version that comes with Performance Lab.

So far, the logic was naively checking for purely the existence of the drop-in via its version constant, without checking whether that version is the latest version though.

Relevant technical choices

  • The logic bails if the PL drop-in is already present and in the latest version (or newer, though that's unlikely).
  • Otherwise, it enters the logic as before. The new change there is that, if there is an existing drop-in, it may be an older version of the Performance Lab one.
    • If the version constant is not set, for sure it's not the PL drop-in.
    • Otherwise, it likely is the PL drop-in, though just to make sure this is verified by checking the file contents.
    • If it is indeed an older version of the PL drop-in, that file is deleted, so that afterwards the previously existing logic can copy over the new drop-in.
  • A new constant PERFLAB_OBJECT_CACHE_DROPIN_LATEST_VERSION contains always the latest version of the drop-in. For that, we need a separate constant as we can't access the PERFLAB_OBJECT_CACHE_DROPIN_VERSION constant defined in the plugin's own object-cache.copy.php file, as that file is never actually loaded.
  • A filter perflab_object_cache_dropin_version is introduced, purely to facilitate unit testing related to the PERFLAB_OBJECT_CACHE_DROPIN_VERSION constant. This filter is needed because constants can't be "un-defined", so code relying on constants is otherwise untestable.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only labels Mar 11, 2024
@felixarntz felixarntz added this to the PL Plugin 3.0.0 milestone Mar 11, 2024
@github-actions
Copy link

github-actions bot commented Mar 11, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@felixarntz felixarntz requested a review from swissspidy March 12, 2024 19:39
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but with some test assertion hardening.

@felixarntz
Copy link
Member Author

PR is updated to address the remaining feedback as well as to cater for the object-cache.copy.php updates merged via #1042.

@felixarntz felixarntz merged commit e8a06e8 into trunk Mar 13, 2024
@felixarntz felixarntz deleted the enhance/server-timing-dropin-update-support branch March 13, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Infrastructure Issues for the overall performance plugin infrastructure [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants