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.


populist’s picture

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

populist’s picture

Another reroll to keep up with -dev.

jec006’s picture

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.

febbraro’s picture

nedjo’s picture

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.

nedjo’s picture

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'));
populist’s picture

new2.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.

febbraro’s picture

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.

populist’s picture

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');
febbraro’s picture

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?

nedjo’s picture

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.

febbraro’s picture

Status:Needs work» Fixed

Thanks for the input here guys. I eventually made it one function in the .module file and the profile.inc loads it if necessary.

Committed. http://drupalcode.org/project/apps.git/commit/d43a25c

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.