Improving theme cloning process in order to minimize manual updates

[shaal]

No description provided.

[ModulesUnraveled]

This makes sense to me, and is definitely a welcome change!

I'd like @ccjjmartin to do the official review since he originally wrote the clone script

[ccjjmartin]

It looks like these two lines should be removed since they are already copied by default when the slim option is not used. See a few lines down where we copy the entire contents of the templates and components directories. This would add these files to the --slim option which is designed not to copy any files from these directories and be a bare bones install. Let me know if you were having some trouble with these files not showing up as expected.

[shaal]

@ccjjmartin, I was not aware of the --slim option.
When I run php emulsify.php -slim myslimtheme I am getting an error -

PHP Warning:  copy(/home/shaal/code/lando/rosenberg/web/themes/contrib/emulsify/components/css): failed to open stream: No such file or directory in /home/shaal/code/lando/rosenberg/web/themes/contrib/emulsify/emulsify.php on line 125
Failed on Phase: Copy files.Your theme was not successfully created.

I think the following line of code is trying to copy a directory that does not exist - components/css
https://github.com/fourkitchens/emulsify/blob/develop/emulsify.php#L495

[ccjjmartin]

@shaal I am curious as to the broken behavior you experienced here. Did "emulsify" on this line get replaced? On the line right after this did it not get replaced? I am definitely pro simplifying install processes so I would love to hear more about the bug you encountered.

[ModulesUnraveled]

I think the goal here is to set this as emulsify by default so that it can be auto-replaced. In the "before" code, it says to manually enter your theme's machine name. Setting it automatically on cloned theme creation is preferred.

[ccjjmartin]

Ok, I was focused on the comment containing the string emulsify but that actually isn't the main change here. I should have been looking at the YOURTHEME versus emulsify. I am good with this change because it makes the out of the box experience better in most scenarios. I will point out that this is not going to work for people who don't put their theme in the root of /themes/ versus /themes/custom/ But that is likely a bigger issue that what we are trying to address here. We should consider replacing this (in another PR not this one) with just a string called PATH and then it drops in the theme path.

Alterations are defined in the _emulsify_get_alterations function which is currently only doing these:

    'Emulsify' => $human_readable_name,
    'emulsify' => $machine_name,
    'Theme that uses Pattern Lab v2' => $description,
    'hidden: true' => '',

It would be pretty easy to add a new line:

    'PATH' => $path,

And add another parameter for to that same function to pass the path in. When I wrote this I wasn't thinking we would need the PATH within the twig files.

[ccjjmartin]

Created this ticket for follow up on this: #271

[shaal]

@ccjjmartin, I updated the code.
Now it doesn't try copying components/css directory
I moved the 2 files that need to be altered to the right place so it works as intended with the -slim option or without.
#268 (commits)

[ccjjmartin]

Ok, I was focused on the comment containing the string emulsify but that actually isn't the main change here. I should have been looking at the YOURTHEME versus emulsify. I am good with this change because it makes the out of the box experience better in most scenarios. I will point out that this is not going to work for people who don't put their theme in the root of /themes/ versus /themes/custom/ But that is likely a bigger issue that what we are trying to address here. We should consider replacing this (in another PR not this one) with just a string called PATH and then it drops in the theme path.

Alterations are defined in the _emulsify_get_alterations function which is currently only doing these:

    'Emulsify' => $human_readable_name,
    'emulsify' => $machine_name,
    'Theme that uses Pattern Lab v2' => $description,
    'hidden: true' => '',

It would be pretty easy to add a new line:

    'PATH' => $path,

And add another parameter for to that same function to pass the path in. When I wrote this I wasn't thinking we would need the PATH within the twig files.

[ccjjmartin]

Good catch, this needed to go.

[ccjjmartin]

@shaal Can you remove these two lines completely and apply this patch to your branch:

diff --git a/emulsify.php b/emulsify.php
index a3bec1c..a889a5e 100644
--- a/emulsify.php
+++ b/emulsify.php
@@ -550,7 +550,7 @@ function _emulsify_get_files_to_rename() {
  * @return boolean
  *   A boolean representing the success or failure of the function.
  */
-function _emulsify_alter_files($theme_path, array $files_to_alter = array(), array $alterations = array(), $absolute = FALSE) {
+function _emulsify_alter_files($theme_path, array $files_to_alter = array(), array $alterations = array(), $absolute = FALSE, int $depth = 0) {
   if (empty($files_to_alter) || empty($alterations)) {
     return TRUE;
   }
@@ -568,11 +568,11 @@ function _emulsify_alter_files($theme_path, array $files_to_alter = array(), arr
       $files = scandir($file_path);
       $files = array_splice($files, 2);
       foreach ($files as $file) {
-        $processed_files[] = $file_path . $file;
-      }
-      $alter_status = _emulsify_alter_files($theme_path, $processed_files, $alterations, TRUE);
-      if ($alter_status === FALSE) {
-        return FALSE;
+        $processed_file = [$file_path . DIRECTORY_SEPARATOR . $file];
+        $alter_status = _emulsify_alter_files($theme_path, $processed_file, $alterations, TRUE, $depth + 1);
+        if ($alter_status === FALSE) {
+          return FALSE;
+        }
       }
     }
     elseif ($file_type === 'file') {

This was originally designed that you can just list the components directory and it recursively searches for files and replaces the text within them. It must have broken somewhere along the way. This fixes it though according to my tests, reproducing steps below:

1. Run php emulsify.php "Alter Test"
2. Verify that the _icon.twig file contains the name alter_test as the machine_name on the desired lines changed above.

NOTE: We may want to check to see if anything else unexpectedly broke since the replacements are now actually working throughout the components and templates directories.

[ccjjmartin]

@shaal Also, there is another instance of components/css present in the file. Can you remove that one too?

[shaal]

@ccjjmartin, Thank you for the new code!
I committed the patch and removed the 2 lines that are no longer needed.
It seems to be working well on my machine.

regarding #268 (comment), I couldn't find any other instance of components/css in emulsify.php

[ccjjmartin]