WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 14 months ago

Last modified 14 months ago

#38673 closed enhancement (fixed)

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 (8)

patch-38673.diff (4.0 KB) - added by MaximeCulea 21 months ago.
patch-38673-1.diff (4.2 KB) - added by MaximeCulea 21 months ago.
patch-38673-2.diff (4.2 KB) - added by MaximeCulea 20 months ago.
patch-38673-3.diff (4.2 KB) - added by MaximeCulea 20 months ago.
patch-38673-4.diff (4.2 KB) - added by MaximeCulea 20 months ago.
38673.diff (4.2 KB) - added by swissspidy 18 months ago.
patch-38673-5.diff (5.5 KB) - added by MaximeCulea 15 months ago.
"wp_disallow_file_mods" > "wp_is_file_mod_allowed"
patch-38673-6.diff (5.5 KB) - added by Mte90 14 months ago.
changed the approach for the new function name

Download all attachments as: .zip

Change History (45)

#1 @MaximeCulea
21 months ago

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

#2 @MaximeCulea
21 months ago

  • Version set to trunk

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


21 months ago

#4 @swissspidy
21 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
21 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
21 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
21 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
21 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 21 months ago by MaximeCulea (previous) (diff)

#9 @swissspidy
21 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
21 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
20 months ago

  • Keywords needs-refresh removed

Much better, thanks!

#12 @Mista-Flo
20 months ago

  • Keywords needs-refresh added

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

#13 @MaximeCulea
20 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
20 months ago

  • Keywords needs-refresh removed

#15 @swissspidy
20 months ago

#38364 was marked as a duplicate.

#16 @MaximeCulea
20 months ago

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

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


20 months ago

#18 @swissspidy
20 months ago

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

#19 @swissspidy
19 months ago

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

#20 @flixos90
18 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
18 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
18 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
18 months ago

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

#24 @MaximeCulea
18 months ago

@swissspidy +1

@swissspidy
18 months ago

#25 @swissspidy
18 months ago

  • Keywords commit added

#26 @swissspidy
16 months ago

  • Component changed from Filesystem API to Upgrade/Install

#27 @swissspidy
16 months 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
15 months 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
15 months 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
15 months 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
15 months ago

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

@MaximeCulea
15 months ago

"wp_disallow_file_mods" > "wp_is_file_mod_allowed"

#32 @MaximeCulea
15 months 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.

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


15 months ago

#34 @swissspidy
15 months ago

@MaximeCulea wp_is_file_mod_allowed() has the opposite meaning of wp_disallow_file_mods() so all the checks would need to be negated, i.e. wp_is_file_mod_allowed() === ! wp_disallow_file_mods()

@Mte90
14 months ago

changed the approach for the new function name

#35 @Mte90
14 months ago

I uploaded a patch with the check of opposite asked by @swissspidy , I hope that is fine. I like to have that feature in the next release :-)

#36 @swissspidy
14 months ago

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

In 40638:

Upgrade/Install: After [40394], rename wp_disallow_file_mods() to wp_is_file_mod_allowed().

This makes it more clear what this function is about.

Props Mte90.
Fixes #38673.

#37 @swissspidy
14 months ago

In 40639:

Upgrade/Install: After [40638], make sure wp_is_file_mod_allowed() actually returns the right value.

See #38673.

Note: See TracTickets for help on using tickets.