Skip to content

Conversation

yonran
Copy link

@yonran yonran commented Sep 2, 2025

Approach

I noticed that on every single wordpress page load, Cloudinary adds quite a bit of overhead (3.4s with xdebug.trace_format=3 enabled; probably 1/4 of that in real life). Of that, 1.1s was get_site_url (which I can optimize on my end), 520ms was telemetry, and 1.2s was Cloudinary\Settings->get_default_settings.

Screenshot 2025-09-02 at 12 32 44 PM

So I asked @openai/codex (gpt-5 high) to optimize the page load.

  • Describe the approach and the suggested implementation.

Remove expensive functionsanitize_file_name from sanitize_slug and make sanitize_slug the identity function. Apparently sanitize_slug serves no purpose.

Proof slugs are controlled (code-defined) and not untrusted user input

  • Params_Trait is mixed into these classes: Settings (php/class-settings.php), Setting (php/settings/class-setting.php), Admin (php/class-admin.php), Assets (php/class-assets.php).
  • Settings/Setting build slugs and storage paths from code (ui-definitions and component registration). See:
    • Settings::__construct() and get_default_settings(): slugs come from arrays defined in ui-definitions/*.php and component params.
    • Connect::register_meta() (php/class-connect.php) registers data options with explicit option_name values (e.g. '_cloudinary_history').
    • Admin::rest_save_settings() only saves whitelisted keys: it filters ->get_params() so that only keys for which ->get_setting(, false) exists are processed. Users never supply arbitrary keys.
  • Assets uses a custom separator '/' and normalizes keys via clean_path(); keys are URL-like but only used in-memory to map asset parents. Both set and get use the same function, so consistency is preserved.
  • After adding in error_log any time sanitize_slug changed anything during plugin init, it found that the following were the only keys that changed. And I found no references in the code to any of those keys without the leading underscore.
    • @data._cloudinary_usage -> @data.cloudinary_usage
    • @data._cloudinary_last_usage -> @data.cloudinary_last_usage
    • @data._cloudinary_settings_version -> @data.cloudinary_settings_version
    • @data._cloudinary_history -> @data.cloudinary_history

Proof no DB migration is necessary

  • Persistence path writes DB option names taken from values, not keys, and never passes through sanitize_slug():
    • Setting::save_value() -> Settings::save_setting() -> Settings\Storage\Options::save().
    • Options::save() calls update_option(->prefix(), ->get(), false).
    • Setting::get_option_name() returns the first segment of the stored 'storage_path' value (php/settings/class-setting.php).
    • Storage::prefix() preserves leading _ option names (php/settings/storage/class-storage.php), so e.g. '_cloudinary_history' stays as-is.
  • Several options are updated directly via update_option() with explicit names in Connect (php/class-connect.php), bypassing Params_Trait entirely.
  • The only thing sanitize_slug() ever changed was ephemeral, in-memory param keys like @data._cloudinary_history -> @data.cloudinary_history. Those params are rebuilt each request and are not persisted to wp_options. Reads and writes always compute the same path on a given request, so changing the sanitizer does not require migrating any persisted structure.

Backwards compatibility / risk assessment

  • In-memory keys under @data, @pending, @submission retain their original segment names (including leading underscores). All callers that set_param/get_param those keys use the same method within the same request, so no mismatch is possible.
  • No user-facing API accepts arbitrary keys; all slugs and option_name values are defined in code.
  • Assets path keys were previously being sanitized per segment; removing that yields exact, normalized paths from clean_path() and improves lookup stability.

Expected impact

  • Removes a hot path during admin init and settings access.
  • No functional change to persisted options or UI behavior.

QA notes

  • Detail the steps needed to verify the PR.

slugs are code-defined and not persisted

Rationale
- Profiler shows heavy time in Params_Trait::sanitize_slug() via sanitize_file_name() (regex, transliteration, filters).
- It is called on every set_param()/get_param() during Settings bootstrap and lookups, even with no user-supplied params.
- Example from profiling: sanitize_slug @data._cloudinary_* being rewritten to @data.cloudinary_* repeatedly.

What changed
- php/traits/trait-params.php: sanitize_slug() now returns the slug unchanged.
- Avoids per-segment sanitize_file_name() work and stops mutating in-memory keys.

Proof slugs are controlled (code-defined)
- Params_Trait is mixed into these classes: Settings (php/class-settings.php), Setting (php/settings/class-setting.php), Admin (php/class-admin.php), Assets (php/class-assets.php).
- Settings/Setting build slugs and storage paths from code (ui-definitions and component registration). See:
  * Settings::__construct() and get_default_settings(): slugs come from arrays defined in ui-definitions/*.php and component params.
  * Connect::register_meta() (php/class-connect.php) registers data options with explicit option_name values (e.g. '_cloudinary_history').
  * Admin::rest_save_settings() only saves whitelisted keys: it filters ->get_params() so that only keys for which ->get_setting(, false) exists are processed. Users never supply arbitrary keys.
- Assets uses a custom separator '/' and normalizes keys via clean_path(); keys are URL-like but only used in-memory to map asset parents. Both set and get use the same function, so consistency is preserved.

Proof no DB migration is necessary
- Persistence path writes DB option names taken from values, not keys, and never passes through sanitize_slug():
  * Setting::save_value() -> Settings::save_setting() -> Settings\Storage\Options::save().
  * Options::save() calls update_option(->prefix(), ->get(), false).
  * Setting::get_option_name() returns the first segment of the stored 'storage_path' value (php/settings/class-setting.php).
  * Storage::prefix() preserves leading '_' option names (php/settings/storage/class-storage.php), so e.g. '_cloudinary_history' stays as-is.
- Several options are updated directly via update_option() with explicit names in Connect (php/class-connect.php), bypassing Params_Trait entirely.
- The only thing sanitize_slug() ever changed was ephemeral, in-memory param keys like '@data._cloudinary_history' -> '@data.cloudinary_history'. Those params are rebuilt each request and are not persisted to wp_options. Reads and writes always compute the same path on a given request, so changing the sanitizer does not require migrating any persisted structure.

Backwards compatibility / risk assessment
- In-memory keys under '@DaTa', '@pending', '@submission' retain their original segment names (including leading underscores). All callers that set/get those keys use the same method within the same request, so no mismatch is possible.
- No user-facing API accepts arbitrary keys; all slugs and option_name values are defined in code.
- Assets path keys were previously being sanitized per segment; removing that yields exact, normalized paths from clean_path() and improves lookup stability.

Expected impact
- Removes a hot path during admin init and settings access.
- No functional change to persisted options or UI behavior.
@aatanasovdev
Copy link
Contributor

Hi @yonran, thank you for suggesting this optimization. We'll review it in detail and follow up.

@aatanasovdev
Copy link
Contributor

aatanasovdev commented Oct 2, 2025

Hi @yonran, the Params_Trait is also used by the Assets class, and it is better to keep the sanitization for that class.

The following changes have been applied: #1101

Can you please test that solution on your side and let us know if it works for your website?

@yonran yonran mentioned this pull request Oct 7, 2025
@yonran
Copy link
Author

yonran commented Oct 7, 2025

Thanks @aatanasovdev! I tested the performance impact of your PR and found it to be just as good as this one.

@aatanasovdev
Copy link
Contributor

Thanks @yonran , I'll close this PR and comment in #1101

Thanks again for your contribution. This performance improvement will be helpful for many websites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants