WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#39006 new defect (bug)

REST API: Language setting should not be present when no languages are available

Reported by: johnbillion Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Options, Meta APIs Keywords: needs-patch
Focuses: rest-api Cc:
PR Number:

Description

In register_initial_settings(), the WPLANG setting is always present (and always available to the REST API), but on the General Settings screen this field is not available if there are no languages installed on the site.

This field should not be registered as an editable setting in the REST API if there are no languages installed on the site, however it makes sense for the setting to be available as read-only, even if it only returns en_US in this situation.

Attachments (2)

39006.diff (2.4 KB) - added by diddledan 3 years ago.
here's an attempt at fixing. I've copied the logic from options-general.php lines 322 thru 327. I'm wondering whether to abstract the logic into a reusable function which both usages call rather than requiring any logic changes to be made in two places.
39006.2.diff (1.4 KB) - added by rmccue 3 years ago.
Include enum of available languages

Download all attachments as: .zip

Change History (24)

#1 @ocean90
3 years ago

but on the General Settings screen this field is not available if there are no languages installed on the site.

That doesn't sound right. It's available when there are language installed (get_available_languages()) or when the translations API returns translations (wp_get_available_translations()). Otherwise the on-the-fly installation of new translations wouldn't work, see [30335].

#2 @johnbillion
3 years ago

Ah yes. Correction: the setting shouldn't be available if the site is not allowing languages to be installed (wp_get_available_translations() returns an empty result), for example via DISALLOW_FILE_MODS.

#3 @jnylen0
3 years ago

Worth noting that we don't currently have a clean way of marking settings as read-only. This is related to a larger question about granularity of the settings endpoint that hasn't really been addressed yet. See also:

An easier fix would be to return an error if an attempt is made to update the setting to an invalid value.

#4 @nacin
3 years ago

@ocean90 @jnylen0 @johnbillion How important is this for 4.7? Seems like we can resolve this better in 4.7.1 or 4.8 and it would not be a problem to do so.

#5 @dd32
3 years ago

I don't see a problem with punting this to 4.7.1 or 4.8. Although I haven't tested it, I assume setting it to a non-existent locale isn't going to be accepted anyway.

#6 @pento
3 years ago

  • Milestone changed from 4.7 to 4.7.1

+1 for punt to 4.7.1.

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 jeffpaul. View the logs.


3 years ago

#9 @rachelbaker
3 years ago

  • Milestone changed from 4.7.1 to 4.7.2

Punting to 4.7.2 - still needs a patch.

@diddledan
3 years ago

here's an attempt at fixing. I've copied the logic from options-general.php lines 322 thru 327. I'm wondering whether to abstract the logic into a reusable function which both usages call rather than requiring any logic changes to be made in two places.

#10 @diddledan
3 years ago

  • Keywords has-patch added; needs-patch removed

@rmccue
3 years ago

Include enum of available languages

#11 @rmccue
3 years ago

  • Keywords needs-patch added; has-patch removed

@diddledan FYI, your patch is saved with an encoding that patch can't understand (at least on my system). Converting to UTF-8 in my editor fixed that, but might want to check settings on your end there :)


wp_get_available_translations() is located in wp-admin/includes/translation-install.php, which makes me think we should move it outside of wp-admin or reconsider using it.

@ocean90 Thoughts on moving it (and hence also translations_api()) out of admin funcs?

Added 39006.2.diff that a) also loads translation-install.php (temporarily! do not commit!), and b) includes an enum containing the available languages. This attachment needs to be redone based on a decision on the above.

#12 follow-up: @rmccue
3 years ago

Re:

I'm wondering whether to abstract the logic into a reusable function which both usages call rather than requiring any logic changes to be made in two places.

I think that makes sense, but we run into a problem here in that there's no good name for the function. We have:

  • get_available_languages - Get list of language codes based on the .mo files.
  • wp_get_installed_translations - Get list of language file data based on .mo file headers.
  • wp_get_available_translations - Get list of language data based on API response.

None of the existing ones can be used authoritatively, and none of the return formats match each other. I can't think of a good name for a new function, this is a bit of a mess really.

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 jnylen. View the logs.


3 years ago

#15 in reply to: ↑ 12 @jnylen0
3 years ago

  • Milestone changed from 4.7.3 to 4.7.4

Replying to rmccue:

this is a bit of a mess really.

Punting to 4.7.4. :)

#16 @swissspidy
3 years ago

I can't think of a good name right now either. There's a reason why naming is one of the hardest things besides cache invalidation :)

Thoughts on moving it (and hence also translations_api()) out of admin funcs?

@ocean90 any thoughts?

#17 @ocean90
3 years ago

wp_get_available_translations() is only required if you want to install new translations. I don't think the REST API should be able to do that as it's quite a slow process.

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


3 years ago

#19 @jnylen0
3 years ago

Given that we lack consensus and an interested committer, and it's been punted through all of the 4.7.x releases so far, I think we should go ahead and punt this to 4.8.

#20 @rmccue
3 years ago

  • Milestone changed from 4.7.4 to 4.8

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


2 years ago

#22 @obenland
2 years ago

  • Milestone changed from 4.8 to Future Release
Note: See TracTickets for help on using tickets.