Method for checking writability of server [#1479164]

I noticed that the module performs a check for writability by doing a call to is_writeable(conf_path()) as part of the apps_profile_authorize_transfer_APPSERVER() functionality and was wondering if that writability check shouldn't be directed at is_writable(drupal_realpath('sites/all/modules') instead since that is where the modules will ultimately get installed.

The attached patch makes that change, but I marked it as needs work/feedback to see the right approach. It also occurred to me that it might be worth modifying the module to install modules in a subdirectory (sites/all/modules/apps) since that allows for slightly more permissions control, but might be blocked by: #986616: Update Manager fails when the primary module/theme for a project lives in a subdirectory.

Comment	File	Size	Author
#7	1479164-apps-has-write-access-check.patch	2.48 KB	populist
#2	1479164-apps-permissions-check-modules-not-conf-reroll.patch	1.24 KB	populist
#1	1479164-apps-permissions-check-modules-not-conf.patch	1.65 KB	populist
	apps-permissions-check-modules-not-conf.patch	1.42 KB	populist

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

populist CreditAttribution: populist commented 12 March 2012 at 19:14

File	Size
1479164-apps-permissions-check-modules-not-conf.patch	1.65 KB

Here is an updated patch using the better patch standards that applies cleanly to -dev.

Comment #2

populist CreditAttribution: populist commented 15 March 2012 at 23:23

File	Size
1479164-apps-permissions-check-modules-not-conf-reroll.patch	1.24 KB

Another reroll to keep up with -dev.

Comment #3

jec006 CreditAttribution: jec006 commented 21 March 2012 at 15:46

Status:

Needs work

» Needs review

I think this patch looks good. I'd like febbraro and e2thex to take a look and make sure this doesn't break any future needs and then we can commit.

Comment #4

febbraro CreditAttribution: febbraro commented 4 May 2012 at 19:18

Possibly related, or might effect this. #1528062: Make installation of downloadables easier using file_unmanaged_copy before FTP/SSH

Comment #5

nedjo

he/him/his

English

CreditAttribution: nedjo commented 21 June 2012 at 22:03

Status:

Needs review

» Reviewed & tested by the community

This fix is indeed needed and correct. It mirrors the proposed writability test in #1555628: Add smart "Verify apps support" screen. If that issue were applied, this patch should be changed to use the new constant APPS_INSTALL_PATH.

Comment #6

nedjo

he/him/his

English

CreditAttribution: nedjo commented 9 July 2012 at 16:58

Status:

Reviewed & tested by the community

» Needs review

There are at least two other directories that Apps might need write access to: sites/all/libraries if it exists or sites/all if not. I suppose we should be checking for these too--which seems to call for a helper method, e.g.,

/**
 * Check whether Apps has write access to libraries and modules directories.
 *
 * @return boolean
 */
function apps_has_write_access() {
  $libraries_dir = is_dir('sites/all/libraries') ? 'sites/all/libraries' ? 'sites/all';
  return is_writable(drupal_realpath($libraries_dir)) && is_writable(drupal_realpath('sites/all/modules'));
}

Comment #7

populist CreditAttribution: populist commented 9 August 2012 at 18:57

File	Size
1479164-apps-has-write-access-check.patch	2.48 KB

Here is a patch that implements the methodology that nedjo presented in #6. In order to get this to cleanly work, I ended up creating two helper functions (one for apps_profile_has_write_access() and one for apps_installer_has_write_access()) since there is inconsistent inclusion of the apps files (some distributions just include the apps.profile.inc file, for example).

I think this patch is really important because the current methodology (checking write-ability of conf_path()) is wrong and Apps doesn't do a great job of failing gracefully.

Comment #8

febbraro CreditAttribution: febbraro commented 9 August 2012 at 20:21

Status:

Needs review

» Needs work

Thanks! One problem is that I don't like that the same function exists in 2 files. I think we should just put it in one place, and either do the include ourself or force the profile/module to update and include the proper file.

Comment #9

populist CreditAttribution: populist commented 9 August 2012 at 20:39

I had a feeling that would be the response. Do you have a preference on where to store these kind of functions?

I also want to make sure that our ultimately solution can handle cases in install profiles that invoke apps through:

// Summon the power of the Apps module
 require_once(drupal_get_path('module', 'apps') . '/apps.profile.inc');

Comment #10

febbraro CreditAttribution: febbraro commented 10 August 2012 at 20:36

The function is general and should be in the apps.module file. Is the apps module not enabled at that time we need it?

Otherwise, the fallback could be for it to live in the profile.inc file and the .module file should just always include that file. Does that make sense?

Comment #11

nedjo

he/him/his

English

CreditAttribution: nedjo commented 10 August 2012 at 20:55

We should avoid loading the profile code on every page since it's never required after initial site install. The .module file seems the right place for this small helper function.