Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#38673 closed enhancement (fixed)

Introduce helper function for DISALLOW_FILE_MODS checks

Reported by: maximeculea's profile MaximeCulea Owned by: swissspidy's profile 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 8 years ago.
patch-38673-1.diff (4.2 KB) - added by MaximeCulea 8 years ago.
patch-38673-2.diff (4.2 KB) - added by MaximeCulea 8 years ago.
patch-38673-3.diff (4.2 KB) - added by MaximeCulea 8 years ago.
patch-38673-4.diff (4.2 KB) - added by MaximeCulea 8 years ago.
38673.diff (4.2 KB) - added by swissspidy 8 years ago.
patch-38673-5.diff (5.5 KB) - added by MaximeCulea 8 years ago.
"wp_disallow_file_mods" > "wp_is_file_mod_allowed"
patch-38673-6.diff (5.5 KB) - added by Mte90 7 years ago.
changed the approach for the new function name

Download all attachments as: .zip

Change History (45)

#1 @MaximeCulea
8 years ago

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

#2 @MaximeCulea
8 years ago

  • Version set to trunk

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


8 years ago

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

#9 @swissspidy
8 years 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
8 years 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
8 years ago

  • Keywords needs-refresh removed

Much better, thanks!

#12 @Mista-Flo
8 years ago

  • Keywords needs-refresh added

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

#13 @MaximeCulea
8 years 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
8 years ago

  • Keywords needs-refresh removed

#15 @swissspidy
8 years ago

#38364 was marked as a duplicate.

#16 @MaximeCulea
8 years ago

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

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


8 years ago

#18 @swissspidy
8 years ago

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

#19 @swissspidy
8 years ago

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

#20 @flixos90
8 years 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
8 years 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
8 years ago

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

#23 @swissspidy
8 years ago

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

#24 @MaximeCulea
8 years ago

@swissspidy +1

@swissspidy
8 years ago

#25 @swissspidy
8 years ago

  • Keywords commit added

#26 @swissspidy
8 years ago

  • Component changed from Filesystem API to Upgrade/Install

#27 @swissspidy
8 years 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 years 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
8 years 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
8 years 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
8 years ago

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

@MaximeCulea
8 years ago

"wp_disallow_file_mods" > "wp_is_file_mod_allowed"

#32 @MaximeCulea
8 years 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.


7 years ago

#34 @swissspidy
7 years 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
7 years ago

changed the approach for the new function name

#35 @Mte90
7 years 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
7 years 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
7 years 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.