WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 6 days ago

#38673 reopened enhancement

Introduce helper function for DISALLOW_FILE_MODS checks

Reported by: MaximeCulea Owned by: swissspidy
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.7
Component: Upgrade/Install Keywords: needs-patch
Focuses: Cc:

Description

Hi guys,

Before submitting a real issue and the according patch, I would like to know how you guys feel about the following situation.

As an agency, I do not allow my clients to update core, plugins, so on. So, I set the DISALLOW_FILE_MODS to true.

But by doing this, no one on site is anymore able to install a new language, translate i18n strings with Locostranslate, for ie, as they are based, and with good reasons, on the DISALLOW_FILE_MODS var. This is due of a maybe file creation.

For now, I described the current and "correct" way of using and doing it. But as far as WordPress evolved since v4.0.0, the available language dropdown and other features are limited as we could say "in the same basket".

My question is, why we couldn't create a new var which affect these languages concern DISALLOW_LANGUAGE_MODS ?

Talking about the core, it would have an effect on :

  • wp_download_language_pack
  • wp_can_install_language_pack

Waiting for you responses ! :raised_hands:

Attachments (7)

patch-38673.diff (4.0 KB) - added by MaximeCulea 6 months ago.
patch-38673-1.diff (4.2 KB) - added by MaximeCulea 6 months ago.
patch-38673-2.diff (4.2 KB) - added by MaximeCulea 6 months ago.
patch-38673-3.diff (4.2 KB) - added by MaximeCulea 6 months ago.
patch-38673-4.diff (4.2 KB) - added by MaximeCulea 6 months ago.
38673.diff (4.2 KB) - added by swissspidy 3 months ago.
patch-38673-5.diff (5.5 KB) - added by MaximeCulea 6 days ago.
"wp_disallow_file_mods" > "wp_is_file_mod_allowed"

Download all attachments as: .zip

Change History (39)

#1 @MaximeCulea
6 months ago

  • Keywords dev-feedback needs-patch 2nd-opinion added
  • Version 4.6.1 deleted

#2 @MaximeCulea
6 months ago

  • Version set to trunk

This ticket was mentioned in Slack in #core-i18n by maximeculea. View the logs.


6 months ago

#4 @swissspidy
6 months ago

Hey there,

Thanks for your report!

There are indeed some valid reasons these functions rely on DISALLOW_FILE_MODS. But before we add another constant, we should rather think about introducing a new wp_disallow_file_mods() function that checks this function and is filterable. Example:

<?php
function wp_disallow_file_mods() { 
    return apply_filters( 'disallow_file_mods', defined( 'DISALLOW_FILE_MODS' ) && DISALLOW_FILE_MODS ); 
}

See #37436 for some patches that partly implemented this already.

This way, one can easily allow file mods under certain circumstances using the filter.

#5 @ocean90
6 months ago

  • Keywords dev-feedback 2nd-opinion removed

Related: #38664

If we'd go with a wp_disallow_file_mods() it should probably get a $context argument.

#6 @MaximeCulea
6 months ago

I like this way to handle this ! Thx for the feedback @ocean90 @swissspidy

Moreover, I agree with @ocean90 as we need to pass a
context var to define where it has been called.

If everyone agrees on this I can supply the patch soon ;)

#7 @swissspidy
6 months ago

Hah, I was actually going to suggest passing the context to the function and the filter as well.

@MaximeCulea Go for it :-)

#8 @MaximeCulea
6 months ago

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

Please find the according patch in attached files : patch-38673.diff.

By applying this patch, people can now filter the DISALLOW_FILE_MODS constant. So, to conclude on the way to add languages, or whatever, while DISALLOW_FILE_MODS is activated, it is only needed to filter wp_disallow_file_mods() like this :

<?php add_action( 'disallow_file_mods_download_language_pack', '__return_false' );

@swissspidy, I am now why wondering about doing this .. because we are like trespassing security behaviour of php constants which could not be redefined, isn't it ?

Last edited 6 months ago by MaximeCulea (previous) (diff)

#9 @swissspidy
6 months ago

  • Keywords needs-refresh added; dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

patch-38673.diff needs to follow the PHP Documentation Standards.

I am now why wondering about doing this .. because we are like trespassing security behaviour of php constants which could not be redefined, isn't it ?

I don't see a problem with that. Constants are not the ideal way to change these things so it makes sense to open it up.

#10 @MaximeCulea
6 months ago

I followed the doc to reformat the filter and add some phpdoc.

Is the following patch-38673-1.diff good now ? @swissspidy

#11 @swissspidy
6 months ago

  • Keywords needs-refresh removed

Much better, thanks!

#12 @Mista-Flo
6 months ago

  • Keywords needs-refresh added

It seems that your last patch miss some lines of the wp_disallow_file_mods function.

#13 @MaximeCulea
6 months ago

Thx @Mista-Flo, had to reinstall git locally cause it crashed on my laptop. As the patch is easy, I just edit it with sublime, but now it is the full one : patch-38673-4.diff !

#14 @MaximeCulea
6 months ago

  • Keywords needs-refresh removed

#15 @swissspidy
5 months ago

#38364 was marked as a duplicate.

#16 @MaximeCulea
5 months ago

@swissspidy, are we good here for merge proposal ? ;)

This ticket was mentioned in Slack in #core-multisite by ocean90. View the logs.


5 months ago

#18 @swissspidy
5 months ago

  • Component changed from I18N to Filesystem API
  • Milestone changed from Future Release to 4.8

#19 @swissspidy
5 months ago

  • Owner set to swissspidy
  • Status changed from new to assigned

#20 @flixos90
3 months ago

Related to this is #39677.

The original goal from this ticket can be achieved via either, but in the end I think both tickets are valid as they provide different kinds of flexibility.

#21 @MaximeCulea
3 months ago

@flixos90 yeap !

You are playing with roles while I am just refactoring way to check existing roles.

Two tickets providing, as you say, two different ways of flexibility :)

#22 @swissspidy
3 months ago

Moving the file mods check to a function is a good step nonetheless. It's not just because of the translations.

#23 @swissspidy
3 months ago

  • Summary changed from Handle languages as 'DISALLOW_FILE_MODS' is activated to Introduce helper function for DISALLOW_FILE_MODS checks

#24 @MaximeCulea
3 months ago

@swissspidy +1

@swissspidy
3 months ago

#25 @swissspidy
3 months ago

  • Keywords commit added

#26 @swissspidy
3 weeks ago

  • Component changed from Filesystem API to Upgrade/Install

#27 @swissspidy
3 weeks ago

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

In 40394:

Upgrade/Install: Introduce wp_disallow_file_mods() helper function.

This is a wrapper around the checks for the DISALLOW_FILE_MODS constant to determine whether file modifications are disallowed.

Props MaximeCulea.
Fixes #38673.

#28 @johnjamesjacoby
8 days ago

Just ran into this, and the function name seems incorrect.

How about wp_is_file_mod_allowed() instead?

(wp_disallow_file_mods() reads (to me) like I would use it to disallow file modifications.)

#29 @MaximeCulea
7 days ago

@johnjamesjacoby I totally agree with you on the name.

But as other constants through helpers, I wanted to keep the same (old) pattern as @swissspidy suggested it for naming the method.

#30 @johnbillion
7 days ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Agreed with j³. The function name should use some form of _is_ terminology.

#31 @MaximeCulea
7 days ago

Okay, going to patch it in the 2-3 next hours, then ! :)

@MaximeCulea
6 days ago

"wp_disallow_file_mods" > "wp_is_file_mod_allowed"

#32 @MaximeCulea
6 days ago

Here we are : patch-38673-5.diff.
I have renamed the func name from "wp_disallow_file_mods" to "wp_is_file_mod_allowed" and added some refactoring/documentation.

Note: See TracTickets for help on using tickets.