Improve how kalatheme detects if modules and libararies paths are writeable

[soniktrooth]

While looking into #123 I came across kalatheme_has_write_access() and kalatheme_lib_dir()

These functions seem a little fuzzy to me and I have questions for @pirog or anyone who knows better.

function kalatheme_has_write_access() {
  return is_writable(drupal_realpath(kalatheme_lib_dir())) && is_writable(drupal_realpath('sites/all/modules'));
}

Why do we ever need to check if the modules dir is writable? What are we writing to the modules dir and why? My understanding was that if your module's dir is writable you got some serious security problems...

function kalatheme_lib_dir() {
  return is_dir('sites/all/libraries') ? 'sites/all/libraries' : 'sites/all';
}

Seems a little hard coded and sledge hammery to me. Panopoly includes the libraries module which can tell us where the libraries dir is. At worst it's probably a site var anyway. Is there a reason we're not doing that?

[andrewmallis]

the subtheme should write to sites/all/themes generally – that said we assume.
$theme_path can return the admin theme, so we might want to use
drupal_get_path('theme', 'kalatheme');

but I am guessing that https://api.drupal.org/api/drupal/includes%21file.inc/function/drupal_realpath/7
may have been used in this instance in order to resolve symlinks, but I do not recall and would defer to pretty much anyone

[soniktrooth]

drupal_get_path() returns a path relative to drupal root, whereas drupal_realpath() returns an absolute path, which I'm assuming is what is_writable() needs.

Regardless of that, my query is more about why we're checking the modules dir instead of the themes dir which is where—as you said—the subtheme should be written to.

[RobLoach]

Writing to a subtheme directory seems strange too. The Libraries module has libraries_get_path(), which can be a nice wrapper around the path/library discovery. We could transition from having our custom implementation to using something like that.

[RobLoach]

Looks like part of that is to update KalaTheme itself, which seems like it would be something for either Drush, or Update module to take on. Any reason for this to be there at all?

[soniktrooth]

@RobLoach KT has to be able to write to the themes dir in order to generate a sub theme. Are you suggesting this is insecure as well? I haven't really explored the drush integration in KT but the functionality to generate a sub theme exists in the config. I don't have experience with the implementing the update module—are we able to get around any security implications by using it?

[andrewmallis]

I too am confused why we are checking sites/all/modules vs sites/all/themes. But again, we should instead check the location of kalatheme, because multisites would have a pattern instead like sites/mysite.com/themes/ or who knows sites/mysite.com/themes/contrib/ (UCSF stack does this).

I have updated the issue description to better reflect our goals.

To address some other notions raised here:

@soniktrooth is correct we cannot use the file_uri_scheme() with is-writable() so yes, drupal's implementation of php's realpath() function is necessary. More insight here: https://api.drupal.org/api/drupal/includes%21file.inc/function/file_prepare_directory/7

Drush subtheme generation is already possible:
https://github.com/drupalprojects/kalatheme/wiki/Installation-with-Drush
but the usecase the functions you question are meant primarily to allow subtheme generation via the UI for non-technical users.

We already require Libraries
https://github.com/drupalprojects/kalatheme/wiki/Setup-and-Installation
so it could be wise as @RobLoach suggests to implement libraries_get_path()

It is pretty amazing that within a single theme so much is possible without other helper modules or themes. Zen had https://www.drupal.org/project/zenophile and Omega https://www.drupal.org/project/omega_tools but recent versions of these themes now only support Drush to generate subthemes.

The subtheme UI generator is a pretty big differentiator, and something we should retain, even if the implementation needs to be a bit hacky in the end.

[soniktrooth]

I'm on board with switching to using libraries_get_path(). I'd still like to hear from @pirog to see if he had any context on why is_writable(drupal_realpath('sites/all/modules')) was needed instead of is_writable(drupal_realpath('sites/all/themes'))

[pirog]

So i think kalatheme_lib_dir() exists for the following reasons

1. To handle the case where sites/all/libraries doesn't exist yet. In which case i think we create it. This is predicated on the assumption that panopoly is not a dependency but only panels, libraries and some other things. If you assume panopoly is a dep then you should be good just checking for s/a/libs
2. libraries_get_path() only returns the directory of a specific library and not the path of the libraries directory itself (this might have changed since ive looked at this). Because this is used in subtheme generation we don't have a lib name to pass in at this point.

WRT kalatheme_has_write_access() i am not completely sure why we are looking at /s/a/modules vs /s/a/themes. It seems like a pretty weak check so i suspect we are doing this only to ascertain whether the user is in SFTP mode or GIT mode on pantheon. Said another way i think we do this basically just to check to see whether we need to use authorize.php to move the files or not.

NOTE: subthemes can also potentially be dropped in an installation profile.

@soniktrooth i hope that helps a bit.

[soniktrooth]

This does help @pirog—thanks. You are right about libraries_get_path it only returns the path to a specific library and upon further inspection I've found that the library module is actually happy for libraries to exist in multiple locations like modules and themes (see libraries_get_libraries()).

I think we should do the following:

rename kalatheme_lib_dir() or just pull the code into kalatheme_has_write_access()

It's only ever called by kalatheme_has_write_access() and of the two possible returns of the function (sites/all/libraries or sites/all) neither constitutes an accurate answer to the question implied by the function name: "Where is the libraries folder".

Add extra comments to kalatheme_has_write_access()

These should explain that the check is more about checking if we can use authorize.php and the updater than it is about whether we have write access to the themes.

[pirog]

@soniktrooth these all seem like very solid improvements!

[soniktrooth]

fix was merged. Closing.