Comments

dkinzer’s picture

Note: even though this patch makes it so that an initial module is generated, the components do not actually get exported until the module is enabled and the drush command is reapplied (clearly that is a bug).

dkinzer’s picture

StatusFileSize
new2.2 KB
new1.82 KB

I'm adding a simple test to this bug report along with the patch.

dkinzer’s picture

Assigned:dkinzer» Unassigned
pfrenssen’s picture

Priority:Normal» Major
Status:Needs review» Needs work

Raising priority on this, the ability to create new features is a core functionality of Features and this is now broken.

Code review:

Coding standards

The patch is not compliant with coding standards, you can use a tool such as the Coder module in combination with PHP Codesniffer to easily detect these (once set up you can just do drush dcs [filename] to give you a list of code standards violations).

--- a/tests/features.test
+++ b/tests/features.test
@@ -280,3 +280,65 @@ class FeaturesCtoolsIntegrationTest extends DrupalWebTestCase {
     }
   }
}
+
+
+
+/**

Adding multiple successive lines of whitespace. This also occurs in other places in the patch.

+class FeaturesDrushExportTest extends DrupalWebTestCase {
...
+      'description' => t('Run functional tests on Features `drush` commands.') ,

There is a space preceding the comma.

+class FeaturesDrushExportTest extends DrupalWebTestCase {
...
+  private $test_module_path = "sites/all/modules/my_sweet_feature";

Use single quotes instead of double quotes. There are also other instances where double quotes are used instead of single quotes.

+  public function setUp() {
+    parent::setUp(array('features',));
+  }

Don't use a trailing comma when defining an array on a single line.

+  public function testModuleExportNew(){

Precede curly braces with a space.

Typos

+  public function testModuleExportNew(){
+    // We need to make sure the module does not exists.

This should be '...does not exist'.

Functionality

In my install the test failed, and when I exported a new feature with a node type the module was created but was missing the node type export.

dkinzer’s picture

@pfrenssen Oops, Thanks for the review. I usually have drupal linting turned on but I guess I forgot to enable it this time (I created the patch on a new environment).

As for the non formatting issue: I do mention in my comment to #1 that I saw this bug as well. I thought of these as two issues, though I guess they are related enough that we can lump them together. The first problem I wanted to solve was just getting the module to export.

On my system once that module has exported the next step was to enable the exported module and run the features export call again. That second attempt exported the actual component. I suppose a similar strategy could be used programmatically. (My suspicion is that the components do not export because somewhere a call is being made to module_exists, which is true only if the module has been enabled).

dkinzer’s picture

@pfressen I ran coder-review and the patch and the there are only 2 issues:

drush coder node-1935876-2.patch
Drupal Coding Standards

b/features.drush.inc: @@ -435,6 +435,7 @@:
No Problems Found

b/tests/features.test: @@ -280,3 +280,65 @@:
+31: [normal] missing space after comma
+42: [normal] use a space between the closing parenthesis and the open bracket

Status Messages:
Coder found 2 patch hunks, 2 normal warnings, 0 warnings were flagged to be ignored

Coder found 2 patch hunks, 2 normal warnings, 0 warnings were flagged to be ignored                    [status]
Drush command terminated abnormally due to an unrecoverable error.                                     [error]

The other formatting issues seem to be part this this should be address at a later time not as part of this issue.

dkinzer’s picture

StatusFileSize
new2.82 KB
new2.05 KB

For this patch pair I ran coder and made some minor formatting changes per @pfrenssen suggestion.

Also I went ahead and added a failing test plus patch for the bug that I describe in comment #1 and that @pfrenssen reiterates in comment #4.

Results for linting for this patch are:

drush coder node-1935876-7.patch
Drupal Coding Standards

b/features.drush.inc: @@ -435,6 +435,15 @@:
No Problems Found

b/tests/features.test: @@ -280,3 +280,69 @@:
No Problems Found

Coder found 2 patch hunks, 0 warnings were flagged to be ignored                                       [status]

dkinzer’s picture

Status:Needs work» Needs review
dkinzer’s picture

StatusFileSize
new2.82 KB
new2.82 KB

Forgot spelling error... As for the other coding standard errors @pfrenssen mentions, I am not getting them when I run drush dcs (perhaps the standard has changed in D7? - I'm using coder version 7.x-2.0-beta2).

dkinzer’s picture

StatusFileSize
new2.82 KB
new2.05 KB

Forgot to include test-only patch.

pfrenssen’s picture

Hey, my review this morning was cut off abruptly by my train entering the station, sorry for the curtness! I was not actually using a review tool, I spotted these myself, I just wanted to suggest using a tool like coder or codesniffer :) Mind that these tools do not detect everything.

Indeed the problem you described in #1 is exactly what I encountered, just an empty skeleton module is created, without any exported components. It should not be necessary to enable the module on export to work around this issue since that might have unwanted side effects (hook_enabled() and hook_install() firing, ...).

Anyway I'm now on my next train journey, I will now review and test your updates. I think the test failures I encountered this morning might have to do with permissions on the sites/all/modules folder, will investigate and report back shortly.

pfrenssen’s picture

Status:Needs review» Needs work
StatusFileSize
new2.94 KB
new2.78 KB

I have been able to fix the failing test about the feature creation by saving the feature in the public:// folder. This works properly on installations that do not allow the web server to write in the sites/all/modules folder. This also has the advantage that the Simpletest creates a different unique folder with each test run, so we can get rid of the code that checks if the module already exists.

I also fixed some minor whitespace and double quotes issues, and improved some wording.

So the next thing is to figure out why the components are not exported.

dkinzer’s picture

Status:Needs work» Needs review
StatusFileSize
new1.75 KB

OK I removed the calls to enable the module and it works but I don't see any way of not exporting to the modules folder. The call to features_load_feature takes a module_name as it's parameter, and as far as I know, drupal cannot load modules outside of the modules folder.

dkinzer’s picture

StatusFileSize
new2.45 KB
new1.75 KB

Forgot to add patch.

pfrenssen’s picture

Why would you need to load the module? The test should verify if the feature is exported correctly, and you can do that perfectly by looking at the exported files in the files folder.

Oh I see you have a call to features_load_feature() in the patch. That should not be there at all. If you are exporting a new feature it does not yet exist, so it cannot possibly be loaded. The web interface also allows to export modules and offer them as a tarred file to the browser, without placing files in the modules folder. Exporting with drush should be no different :)

dkinzer’s picture

StatusFileSize
new6.16 KB
new1.83 KB

The following patch reverts back to exporting to the files folder and is able to export without having to rely on features_load_feature() as per @pfrenssen comment 15.

dkinzer’s picture

StatusFileSize
new6.12 KB
new1.83 KB

Removed some dev code and cleaned up test.

pfrenssen’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new3.47 KB
new6.03 KB

Excellent work!! It works beautifully now.

I have made some minor changes to the patch as a final cleanup:

  • Renamed _features_generate_export() to _drush_features_generate_export() to be consistent with other functions in this file.
  • Put the private variables used in the test inside the test itself, they are only used here.
  • Generate a random name for the feature in the test, as is the custom :)
  • The variable names for the module directory and the destination directory were a bit ambiguous so I renamed them to $destination and $directory.

These changes do not really impact the patch, so I assume it's OK for me to RTBC this.

dkinzer’s picture

@pfrenssen, thanks! Hopefully this will get committed upstream soon.

mpotter’s picture

Status:Reviewed & tested by the community» Fixed

Nice work on this one. The commit mentioned in the OP that removed this was a long time ago to combine the fa and fe commands and I'm surprised this didn't get mentioned before.

Seems to be working here, so I committed this to 4efb56d.

nicholasruunu’s picture

This now works with the dev version, but not if you're doing:

drush fe custom/featurename node:page

pfrenssen’s picture

@nicholasruunu, can you create a new issue for that?

dkinzer’s picture

@nicholasruunu I believe you need to pass the --destination parameter if you want to export to somewhere other than the default module path.

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

Elijah Lynn’s picture

Issue summary:View changes

Added hyperlink to commit.