WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#40812 closed enhancement (fixed)

Allow wp-login.php page title separator to be filtered

Reported by: henry.wright Owned by: SergeyBiryukov
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords:
Focuses: Cc:

Description

The wp-login.php page title separator is currently hard coded as either › or ‹ depending on is_rtl().

Attachments (4)

40812.patch (509 bytes) - added by nishitlangaliya 3 years ago.
Fixed #40812
40812-document_title_separator.patch (544 bytes) - added by nishitlangaliya 3 years ago.
Fixed-40812 using document_title_separator filter
40812-wp_get_document_title.patch (520 bytes) - added by nishitlangaliya 3 years ago.
fixed - 40812 using wp_get_document_title
40812-document_title_separator-New.patch (706 bytes) - added by nishitlangaliya 3 years ago.
Fixes #40812 using document_title_separator

Download all attachments as: .zip

Change History (23)

#1 @henry.wright
3 years ago

  • Component changed from General to Login and Registration

@nishitlangaliya
3 years ago

Fixed #40812

#2 @nishitlangaliya
3 years ago

  • Keywords has-patch added

#3 follow-up: @SergeyBiryukov
3 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to 4.9

I don't see how 40812.patch is relevant here.

We should either apply the document_title_separator filter to the $separator variable, or (preferably) make the login page use wp_get_document_title(), which would solve both this ticket and #40814.

The latter might be a bit tricky, because wp_get_document_title() doesn't accept a predefined title like login_header() does, but we could probably work around that using document_title_parts filter.

Moving to the milestone to handle at the same time as #40814.

#4 @henry.wright
3 years ago

I agree with @SergeyBiryukov that 40812.patch doesn't quite address the issue. The 2 solutions put forward in #3 seem more appropriate.

#5 @nishitlangaliya
3 years ago

@SergeyBiryukov ,

Thanks for your reply let me try as per your suggestion :)

@nishitlangaliya
3 years ago

Fixed-40812 using document_title_separator filter

@nishitlangaliya
3 years ago

fixed - 40812 using wp_get_document_title

#6 @nishitlangaliya
3 years ago

  • Keywords has-patch added; needs-patch removed

@SergeyBiryukov ,

I have attached two patch as which is below:

  1. 40812-document_title_separator.patch - Try to fix issue using document_title_separator filter.
  2. 40812-wp_get_document_title.patch - Try to fix issue using wp_get_document_title function

Please review it when you get a chance and let me know if any changes required.

Thanks

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


3 years ago

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


3 years ago

#9 @swissspidy
3 years ago

40812-wp_get_document_title.patch is only half of the solution because of the earlier comment:

The latter might be a bit tricky, because wp_get_document_title() doesn't accept a predefined title like login_header() does, but we could probably work around that using document_title_parts filter.

We'd need to hook in to the document_title_parts filter to make sure the login title is correct.

If we were to go with 40812-document_title_separator.patch, the filter needs to be documented like so:

/** This filter is documented in wp-includes/general-template.php */

#10 @henry.wright
3 years ago

40812-document_title_separator.patch seems to use a hyphen for the separator. I believe the actual separator character used in WordPress page titles is –.

@nishitlangaliya
3 years ago

Fixes #40812 using document_title_separator

#11 @nishitlangaliya
3 years ago

@swissspidy , @henry.wright

Thanks for your suggestions and as per your suggestions I have updated my patch. Please check for the same and let me know if there is change required.

Fixes #40812 using document_title_separator

Thanks.

#12 @henry.wright
3 years ago

  • Keywords dev-feedback added

@swissspidy @SergeyBiryukov @nishitlangaliya

What are your thoughts on modifying wp_get_document_title() to accept a default title? We can then pass it the first param of login_header(). This will avoid the need to filter document_title_parts.

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


3 years ago

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


3 years ago

#15 @SergeyBiryukov
3 years ago

  • Milestone changed from 4.9 to Future Release

#16 in reply to: ↑ 3 @SergeyBiryukov
3 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

Replying to SergeyBiryukov:

We should either apply the document_title_separator filter to the $separator variable, or (preferably) make the login page use wp_get_document_title(), which would solve both this ticket and #40814.

On second thought, both document_title_separator and wp_get_document_title() only apply to front-end pages.

I think login page is a part of the admin rather than front-end. Admin pages use ‹ as a separator, and it's a part of a translatable string, so that RTL locales could translate it to ›.

We could use the same approach on login page and apply the admin_title filter there, but that would lead to fatal errors if a plugin calls get_current_screen() on admin_title, because it's not available on login page.

So I guess a new filter, login_title, should be introduced.

#17 @SergeyBiryukov
3 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 41691:

Login and Registration: Introduce login_title filter for the <title> tag content on login page.

The new filter mirrors the admin_title filter used on admin pages.

Props nishitlangaliya, henry.wright, SergeyBiryukov.
Fixes #40812.

#18 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 4.9

#19 @SergeyBiryukov
3 years ago

  • Keywords needs-patch removed
Note: See TracTickets for help on using tickets.