Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Trying to create a new feature module using `drush fe` fails because pertinent code was removed by commit a48b0a47af58f0afd32984410f49e625e05e723d. I am attaching a patch that fixes the problem.
Comment | File | Size | Author |
---|---|---|---|
#18 | 1935876-18-features-drush_fe.patch | 6.03 KB | pfrenssen |
#18 | interdiff.txt | 3.47 KB | pfrenssen |
#17 | node-1935876-17-test-only.patch | 1.83 KB | dkinzer |
#17 | node-1935876-17.patch | 6.12 KB | dkinzer |
#16 | node-1935876-16-test-only.patch | 1.83 KB | dkinzer |
Comments
Comment #1
dkinzer CreditAttribution: dkinzer commentedNote: 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).
Comment #2
dkinzer CreditAttribution: dkinzer commentedI'm adding a simple test to this bug report along with the patch.
Comment #3
dkinzer CreditAttribution: dkinzer commentedComment #4
pfrenssenRaising 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).Adding multiple successive lines of whitespace. This also occurs in other places in the patch.
There is a space preceding the comma.
Use single quotes instead of double quotes. There are also other instances where double quotes are used instead of single quotes.
Don't use a trailing comma when defining an array on a single line.
Precede curly braces with a space.
Typos
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.
Comment #5
dkinzer CreditAttribution: dkinzer commented@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 istrue
only if the module has been enabled).Comment #6
dkinzer CreditAttribution: dkinzer commented@pfressen I ran coder-review and the patch and the there are only 2 issues:
drush coder node-1935876-2.patch
Drupal Coding Standards
The other formatting issues seem to be part this this should be address at a later time not as part of this issue.
Comment #7
dkinzer CreditAttribution: dkinzer commentedFor 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:
Comment #8
dkinzer CreditAttribution: dkinzer commentedComment #9
dkinzer CreditAttribution: dkinzer commentedForgot 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).
Comment #10
dkinzer CreditAttribution: dkinzer commentedForgot to include test-only patch.
Comment #11
pfrenssenHey, 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.
Comment #12
pfrenssenI 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.
Comment #13
dkinzer CreditAttribution: dkinzer commentedOK 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.Comment #14
dkinzer CreditAttribution: dkinzer commentedForgot to add patch.
Comment #15
pfrenssenWhy 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 :)
Comment #16
dkinzer CreditAttribution: dkinzer commentedThe 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.
Comment #17
dkinzer CreditAttribution: dkinzer commentedRemoved some dev code and cleaned up test.
Comment #18
pfrenssenExcellent work!! It works beautifully now.
I have made some minor changes to the patch as a final cleanup:
These changes do not really impact the patch, so I assume it's OK for me to RTBC this.
Comment #19
dkinzer CreditAttribution: dkinzer commented@pfrenssen, thanks! Hopefully this will get committed upstream soon.
Comment #20
mpotter CreditAttribution: mpotter commentedNice 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.
Comment #21
nicholasruunu CreditAttribution: nicholasruunu commentedThis now works with the dev version, but not if you're doing:
drush fe custom/featurename node:page
Comment #22
pfrenssen@nicholasruunu, can you create a new issue for that?
Comment #23
dkinzer CreditAttribution: dkinzer commented@nicholasruunu I believe you need to pass the --destination parameter if you want to export to somewhere other than the default module path.
Comment #24.0
Elijah LynnAdded hyperlink to commit.