WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 44 hours ago

#51513 new defect (bug)

Rename 'wp_is_application_passwords_available' filter and function

Reported by: SergeyBiryukov Owned by:
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch
Focuses: Cc:

Description

Background: #42790, #50428.

The current name of wp_is_application_passwords_available doesn't read like correct English to me :)

I believe it should be wp_application_passwords_enabled, for consistency with other similar filters in core.

This would also be more consistent with the strings displayed in the UI:

  • "Application passwords are not enabled."
  • "Application passwords are not enabled for your account. Please contact the site administrator for assistance."

The same applies to wp_is_application_passwords_available_for_user.

For reference, here's the list of filters and options with a similar purpose:

  • plugins_auto_update_enabled
  • themes_auto_update_enabled
  • rest_enabled
  • rest_jsonp_enabled
  • xmlrpc_enabled
  • wp_fatal_error_handler_enabled
  • wp_lazy_loading_enabled
  • global_terms_enabled
  • link_manager_enabled
  • wp_sitemaps_enabled

Attachments (1)

51513.diff (5.8 KB) - added by SergeyBiryukov 3 weeks ago.

Download all attachments as: .zip

Change History (6)

@SergeyBiryukov
3 weeks ago

#1 follow-up: @TimothyBlynJacobs
3 weeks ago

I'm not strongly opposed, but the reason I went with available was to differentiate between the user having enabled it for their account. App Passwords might be available to the user, but they might not have enabled it.

#2 @ocean90
2 weeks ago

I also think that we should stick to available because wp_is_application_passwords_available() explicitly checks for something that needs to exist to make application passwords available. Similar to the now deprecated rest_enabled filter, we shouldn't use a name which promotes that it can only be used to disable application passwords.

Looking at the UI strings, "Application passwords are not enabled." should probably be changed to clarify that the requirements are not satisfied which can also be represented by the HTTP status code 501 instead of 500.

#3 in reply to: ↑ 1 @SergeyBiryukov
11 days ago

Thanks for the additional context!

I think, at the very least, "is" should be dropped:

  • wp_application_passwords_available()
  • wp_application_passwords_available_for_user()

That would be more consistent with other filters. I realize that "Application Passwords" is the name of the feature, but without that context "is" looks a bit weird to me in these names.

Still, wp_application_passwords_available_for_user() is confusing to me. With the "available" vs. "enabled" difference in mind, it's hard to tell what exactly if tests for:

  • Is it for application passwords being available for the user, but not necessarily enabled?
  • Or is it for application passwords being enabled for the user?

The description says:

Checks if Application Passwords is enabled for a specific user.

So it seems like wp_application_passwords_enabled_for_user() would be a more accurate name?

#4 @TimothyBlynJacobs
11 days ago

That would be more consistent with other filters. I realize that "Application Passwords" is the name of the feature, but without that context "is" looks a bit weird to me in these names.

I guess to me wp_is_application_passwords_available reads like the question in English "Is application passwords available?". To me that isn't unclear. Whereas "Application passwords available?" reads like a disjointed question.

Is it for application passwords being available for the user, but not necessarily enabled?

It is for this. The code doesn't look to see if the user has any app passwords created.

So it seems like wp_application_passwords_enabled_for_user() would be a more accurate name?

IMO the description should be updated.

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


44 hours ago

Note: See TracTickets for help on using tickets.