Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#46312 closed defect (bug) (fixed)

Replace http with https in custom links menu item

Reported by: aksl95's profile aksl95 Owned by: audrasjb's profile audrasjb
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-screenshots has-patch
Focuses: Cc:

Description

On the URL input of custom links menu item, replace the value http with https.

Attachments (9)

46312-patch-https.diff (917 bytes) - added by aksl95 6 years ago.
custom-link-gutenberg-example.png (18.5 KB) - added by audrasjb 6 years ago.
Gutenberg example
custom-link-beforepatch.png (21.9 KB) - added by audrasjb 6 years ago.
Custom link before patch
custom-link-afterpatch.png (22.2 KB) - added by audrasjb 6 years ago.
Custom link after patch
46312.diff (1.2 KB) - added by mukesh27 6 years ago.
Patch. Check is_ssl() function for https also add placeholder for custom link URL
46312.2.diff (973 bytes) - added by mukesh27 6 years ago.
Updated Patch.
46312.3.diff (2.5 KB) - added by audrasjb 6 years ago.
Adds https to both menus and customizer, replaces value withplaceholder and updates the custom link check
46312.4.diff (3.8 KB) - added by aksl95 6 years ago.
Edit placeholder in javascripts files (nav-menu & customizer). Update the custom link check (http & https).
46312.5.diff (3.8 KB) - added by audrasjb 5 years ago.
patch refreshed against trunk

Download all attachments as: .zip

Change History (44)

#1 @audrasjb
6 years ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Hi @aksl95 and welcome to WordPress Trac

That seems relevant to me.
The existing placeholder can make think that the user must use http. We should encourage the use of https.
Also, the block editor uses https for its placeholders. Let's add some consistency :)

Auto assigning for review.

#2 @audrasjb
6 years ago

  • Keywords has-patch added

@audrasjb
6 years ago

Gutenberg example

@audrasjb
6 years ago

Custom link before patch

@audrasjb
6 years ago

Custom link after patch

#3 @audrasjb
6 years ago

  • Keywords has-screenshots added
  • Milestone changed from Awaiting Review to 5.2
  • Status changed from reviewing to accepted

Wondering is we should also fix customizer placeholder/value in this patch or open a new one.
Maybe we should also use placeholder attribute instead of value.

@mukesh27
6 years ago

Patch. Check is_ssl() function for https also add placeholder for custom link URL

#4 @audrasjb
6 years ago

  • Keywords needs-refresh added

Hi @mukesh27 thanks for the update :)

Changing value to placeholder attribute is nice, but I don't think we should check is_ssl() since Custom links can handle external links as well. We should let https by default in my opinion (as a placeholder) ;-)

#5 @mukesh27
6 years ago

@audrasjb, Using is_ssl() we can know if user use SSL or not. if we added https then all custom links goes to it's https page but without SSL website will show https error or move that link using 301 redirection.

Version 0, edited 6 years ago by mukesh27 (next)

#6 @audrasjb
6 years ago

@mukesh27 if it’s a placeholder (which is fine), we don't care so much if the website uses https or not: we are only giving an indication with this placeholder so the user can do whatever they want (http/https) :-)

@mukesh27
6 years ago

Updated Patch.

#7 @mukesh27
6 years ago

  • Keywords needs-refresh removed

Updated patch as per @audrasjb suggestion.

#8 @audrasjb
6 years ago

Related: #46317

Thanks for the update @mukesh27 !

#9 follow-up: @afercia
6 years ago

Worth considering while in Gutenberg https:// is a placeholder, in the menus it's actually part of the input value.

What happens if users just complete the existing https:// with the rest of the URL and the linked website is still on http:// ?

#10 @swissspidy
6 years ago

#46317 was marked as a duplicate.

#11 in reply to: ↑ 9 @audrasjb
6 years ago

Replying to afercia:

Worth considering while in Gutenberg https:// is a placeholder, in the menus it's actually part of the input value.

What happens if users just complete the existing https:// with the rest of the URL and the linked website is still on http:// ?

This is part the purpose of https://core.trac.wordpress.org/attachment/ticket/46312/46312.2.diff:

  • replace http:// with https://
  • use placeholder attribute instead of value
Last edited 6 years ago by audrasjb (previous) (diff)

This ticket was mentioned in Slack in #core by aksl95. View the logs.


6 years ago

#13 @SergeyBiryukov
6 years ago

  • Keywords commit added

#14 @celloexpressions
6 years ago

  • Keywords needs-patch added; has-patch commit removed

The (original) design intent of this "placeholder" is actually to be a part of a value as opposed to a true placeholder. It's provided as a staring point for users that type in a URL. Users pasting in a URL would need to manually remove this default value. I'd prefer keeping the current behavior unless research supports making a change. However, I support updating to https:// either way. I also recall some menu customizer JS that may be tied to the default value and that should be confirmed before making the change.

This needs to be updated both in the customizer and in the admin version of menus if a change is made, either way. It looks like we need a combination of 46312.2.diff and 46312-patch-https.diff once a decision is made on changing from a default value to a placeholder.

@audrasjb
6 years ago

Adds https to both menus and customizer, replaces value withplaceholder and updates the custom link check

#15 @audrasjb
6 years ago

Thanks for your comment @celloexpressions
I don't think value is a good idea since this attribute is supposed to specifiy a default value. Placeholders should be used to specify an example value.

By the way, I updated the patch.
In 46312.3.diff:

  1. Add https to both menu and customizer screens
  2. Replace value with placeholder
  3. Updates the custom link check to handle both http://, https:// and empty value… though it should be empty all the time now we don't have any default value anymore. But… worth it :)

@SergeyBiryukov if you have a chance, can you please check what I say in the item 3) above in the last patch 46312.3.diff ?

#16 @audrasjb
6 years ago

  • Keywords has-patch added; needs-patch removed

#17 @SergeyBiryukov
6 years ago

  • Keywords needs-refresh added
  • Milestone changed from 5.2 to 5.3

Found some associated JS code referring to http:// in nav menus that should be updated as well:

  • js/_enqueues/lib/nav-menu.js
  • js/_enqueues/wp/customize/nav-menuslib/nav-menu.js

With 5.2 Beta 1 deadline approaching in a few hours, I'd rather test this carefully early in 5.3.

@aksl95
6 years ago

Edit placeholder in javascripts files (nav-menu & customizer). Update the custom link check (http & https).

#18 @aksl95
6 years ago

Thanks for you analysis @SergeyBiryukov.

In 46312.4.diff, I updated javascript files.

After submission, I reset the value and update the placeholder with https.

Last edited 6 years ago by aksl95 (previous) (diff)

This ticket was mentioned in Slack in #core by aksl95. View the logs.


6 years ago

#20 @SergeyBiryukov
6 years ago

  • Keywords needs-testing added; needs-refresh removed

#21 follow-up: @jorbin
6 years ago

I wonder if rather than defaulting to https, it might make sense to default to whatever protocol the site is set to use in home.

#22 in reply to: ↑ 21 ; follow-up: @aksl95
6 years ago

Replying to jorbin:

I wonder if rather than defaulting to https, it might make sense to default to whatever protocol the site is set to use in home.

For custom links, I don't know if it has a relationship because these links can be external.

Last edited 6 years ago by aksl95 (previous) (diff)

#23 @audrasjb
5 years ago

#47707 was marked as a duplicate.

#24 in reply to: ↑ 22 @audrasjb
5 years ago

  • Keywords commit added; needs-testing removed

Replying to aksl95:

Replying to jorbin:

I wonder if rather than defaulting to https, it might make sense to default to whatever protocol the site is set to use in home.

For custom links, I don't know if it has a relationship because these links can be external.

Agree. There is no relationship with the website protocol since custom links are used for both internal and external link.

Also, the patch is tested and still applies cleanly on my side. Adding commit keyword.

#25 @audrasjb
5 years ago

For information, the other related "https placeholders" ticket was committed this week: #46320

#26 @audrasjb
5 years ago

  • Keywords needs-refresh added; commit removed

The patch doesn't applies cleanly anymore.

#27 @audrasjb
5 years ago

  • Keywords commit added; needs-refresh removed

Patch refreshed in 46312.5.diff.

Would be great to get a commit action since enhancements should be committed before beta 1 and as this tickets goes with #46320 which is already committed to 5.3.

@SergeyBiryukov if you have a chance… :-)

@audrasjb
5 years ago

patch refreshed against trunk

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


5 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


5 years ago

#30 @audrasjb
5 years ago

  • Type changed from enhancement to defect (bug)

Moving to bugs list to see if it can land along with #46320.

#31 @audrasjb
5 years ago

(it doesn't make a lot of sense to ship #46320 without this one)

#32 @whyisjake
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 46330:

Menus: Replace http with https in placeholders.

Encourage the use of https with the placeholder text in menus.

Fixes #46312
Props aksl95, audrasjb, celloexpressions, SergeyBiryukov, jorbin.

#33 @audrasjb
5 years ago

  • Keywords commit removed

#34 @SergeyBiryukov
5 years ago

#48326 was marked as a duplicate.

This ticket was mentioned in Slack in #core-media by sabernhardt. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.