WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 2 months ago

#52886 new enhancement

Update esc_url to allow for specifying an https:// as default protocol

Reported by: mkaz Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch needs-unit-tests needs-dev-note
Focuses: Cc:

Description (last modified by SergeyBiryukov)

If no protocol is specified for esc_url the function will automatically prepend the http:// protocol. This is likely now the wrong assumption, but potentially can break backwards compatibility if changed, since developers may rely on this.

So this change proposes an additional parameter to the function to specify a default protocol, keeping the old default but now allowing for one to ask for https://

This came up in this ticket: https://github.com/WordPress/gutenberg/pull/30100

The usage could then be:

esc_url( $url, null, 'display', 'https://' );

Attachments (3)

52886.diff (2.2 KB) - added by mkaz 4 months ago.
52886.1.diff (2.3 KB) - added by audrasjb 3 months ago.
52886.2.diff (2.9 KB) - added by mkaz 7 weeks ago.
Unit test added.

Download all attachments as: .zip

Change History (11)

@mkaz
4 months ago

#1 @rachelbaker
4 months ago

  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.8

#2 @SergeyBiryukov
4 months ago

Thanks for the patch!

Just noting that the @since tag should be in the function DocBlock, after @since 2.8.0, and should follow this format for consistency:

@since 5.8.0 Added the `$default_protocol` parameter.

@audrasjb
3 months ago

#3 @audrasjb
3 months ago

  • Keywords needs-dev-note added

In 52886.1.diff:

  • Add @since mention
  • Few coding standards fixes (whitespaces inside docblocks)

Question: shouldn't we also add the new parameter in the clean_url filter at the bottom of the function? Not sure it's really needed, but I prefer to ask :)

#4 @SergeyBiryukov
3 months ago

The usage could then be:

esc_url( $url, null, 'display', 'https://' );

Looking at this again, I wonder if it's straightforward enough in practice, as it would require passing four parameters to every esc_url() call that might be missing the protocol and should default to https://. If the site is known to use https://, that seems redundant.

Perhaps this should be a filter instead, like esc_url_default_protocol? That way it could only be specified once.

Last edited 3 months ago by SergeyBiryukov (previous) (diff)

#5 @SergeyBiryukov
3 months ago

  • Description modified (diff)

#6 @mkaz
3 months ago

Thanks for the follow up on this one, it had fallen off my radar.

My potential concern with a filter is that it would apply to all esc_url() calls.

The use case we had was to implement on the Block Editor for the social links, but if we did it as a filter and once it got to core it the filter would be implemented for everyone which is the same as default to https:// everywhere.

Also, as a filter someone else could override and then it would not pick the right URL for say https://twitter.com/

@audrasjb I'm not sure it makes sense to pass in to the clean_url filter, I can't think of a scenario where it would help.

My other thought was should we change to esc_url_raw to also support defaulting to https://

This ticket was mentioned in Slack in #core-editor by mkaz. View the logs.


2 months ago

#8 @desrosj
2 months ago

  • Milestone changed from 5.8 to Future Release

As we're at feature freeze for 5.8 and this one still needs unit tests, I'm going to punt. Please do continue to discuss and feel free to move back into a numbered milestone once tests are able to be added.

@mkaz
7 weeks ago

Unit test added.

Note: See TracTickets for help on using tickets.