Posted by mojzis on October 10, 2009 at 6:22pm
| Project: | Patterns |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | apotek |
| Status: | active |
| Issue tags: | drush patterns |
Issue Summary
Hi,
I think this module would be even greater with support for drush, don't you ?
thanks
Comments
#1
It's a good idea and needs to be done. We've discussed it a few times and would like to add support but haven't gotten around to it just yet.
Any patches to add support for Drush are welcome and will certainly be committed quickly! :-)
#2
Attached is a preliminary version of drush support. I'll continue the work on next days.
#3
Updated code, just a few bug fixes and improvements. Actually fixed the run/enable command. It now works fine, I am integrating this in our suite of bash tools.
Did you continue working on it?
#4
I tested this some time ago and everything worked nicely except for the actual running the patterns. I haven't gotten around to testing further. I was just talking with ShaneN in IRC and he's working on this as well.
Jonhattan and Verot, have you worked on this further since posting the patch?
#5
I'm going to take a look at the proposed file, but am attaching the drush support version I just roughed in last Friday, in case it generates anything positive.
Obviously there are things I still don't understand, such as how patterns actually does or does not support yaml files. Perhaps it's an issue of documentation.
Attaching my version of a patterns.drush.inc file.
#6
Noticed in the older, more complete (and clean!) version, that the deprecated form of the commands (lacking the hyphen) is in use. it seems the more recent versions of drush require the hyphen syntax. I'll try to smoosh these together
$ drush -l hostname patterns listNotice: "patterns list" must be renamed to "patterns-list" in order to [error]
work with this version of drush. If you are the maintainer for the
module that defines this command, please rename it immediately. If you
are a user of this command, you may enable spaces in commands for a
while by setting "allow-spaces-in-commands" in your drush configuration
file. See example.drushrc.php.
#7
Okay, I merged the older version with my changes, and updated the API to what drush expects now.
We still can't get the drush patterns-enable command to work. It appears to work (we get success messages), but the status of the pattern is not changed to 1. Not sure why.
Would love to have some comments/corrections/insights from anyone who feels so inclined. Thanks.
Also (and this might be a bug in drush), I'm getting some odd namespace conflict with the pm module. When I run drush patterns-enable, for some reason, the arguments, after running the drush enable command, get passed on to drush pm-enable.
#8
Thanks for consolidating the patches. I'm going to be away for the rest of the week and I will be able to check it out only when I come back. Looking forward to it.
#9
I had a look at the patch last week and mainly got stuck at trying to make patterns-enable command to work. Part of our problem is explained here #638712: Non-progressive batch operations not possible. After applying the workaround described in this issue, I was able to run the pattern but still run into quite a few issues mainly related to the fact that patterns need to run each action as a new page request to make sure all static caches are reset and config elements created during patterns execution are recognized and available for all following actions. Unfortunately I never managed to continue my work on fixing those since then.
#10
I did contribute a buggy patch in #2 (just wrong file attached) and then detached off this issue. Sorry for that. I do not use patterns (still) but some day I'd like to test it intensively.
For the batch problem: drush implements its own batch api. This may be the solution to the problem here. Perhaps it needs some changes in the module code. Here is the reference:
http://drupalcode.org/viewvc/drupal/contributions/modules/drush/includes...
#11
jonhattan, thank you very much for your initial patch. It definitely helped moving this issue forward :)
My previous comment was not very detailed but I actually did use drush batch API. Still there are few actions that fail to be executed properly. As I said I didn't get a chance to look at it further.
Whenever I get some time I'm planning to commit patch from #7 along with my code so that others can chip in with their contributions.
#12
Very helpful.
I suppose what we could do in the Patterns module is sniff for an environment variable $_SERVER['IS_DRUSH'] set from the drush patterns extension, and then fork the code to either use the drupal batch api or the drush batch api as necessary.
#13
I'm going to prepare a new version with the misunderstood yaml import stuff ripped out (I will, like the module, only support xml import).
The main and top priorities with this extension are, in my opinion:
1) Being able to run drush patterns-enable
2) Being able to import and run drush patterns-enable in one move.
The use case I see most relevant is for the sysadmin who wants to be able to copy over a patterns file *anywhere* on the file system, and just shoot it into his or her drupal platform via a command line script.
Are there any other top use cases that might knock 1 and 2 off the top?
thanks all.
#14
Those would be the same top priorities for me as well. I would also add a third priority:
3) Being able to generate patterns from Drush
There are a few related issues that could help towards that goal as well:
#514234: Patch for Generating Variable, Role, and Perm Patterns from existing site.
#789830: Addition to patterns form helper
My thinking along these lines would be that you could have something similar to the below commands that would either output to STDOUT or to a file (with an option to append vs, overwrite.) It could be made to take the form id and create the necessary pattern code for you. While dealing with only form ids isn't ideal, at least it would be a start. Ideally it should also have an option to choose the preferred format.
$ drush patterns-generate --format=yaml --append form_idor
$ drush patterns-generate --pipe --format=xml form_id
Down the road when we have better pattern building and generation support (more comprehensive and using native syntax) it then could be added to Drush as well.
#15
I've done some sniffing around. There are a few built-ins that *might* suffice for sniffing out whether the code is running through drush or through mod_php server request:
- = array key does not exist
These look like the most predictable or solid switches to test for.
However, the cleanest, most explicit way to do this is probably for me to add:
define('PATTERNS_IS_DRUSH', TRUE);
to the top of the drush.patterns.inc file.
Ok. So in whatever way I would implement such a test, it looks to me like the batch code is deeply tangled into lots of different code blocks in the patterns.module. It would be fantastic if we could create some kind of clean wrapper interface, like so:
class PatternsBatch
{
}
class PatternsBatchDrush extends PatternsBatch
{
//override the methods that need to be handled differently in the case of drush.
}
then, in the patterns module, we init with
if (PATTERNS_IS_DRUSH)
batch_processor = new PatternsBatchDrush();
else
batch_processor = new PatternsBatch();
And then in the code we just do
batch_processor->method();
that way we don't need a hundred if/then blocks to allow drush to integrate with the module.
I guess it should be obvious by now that I think forking the code into drush-specific functions sounds like a maintenance nightmare.
#16
Ok. Thinking more about forking the code.
In patterns_execute_pattern(), the third param is the $mode param (which can only be set to 'batch' at the moment).
Another way to do this is to add a function to the drush.patterns.inc called patterns_execute_patterns_drushbatch(). This would be a fork of the patterns_execute_pattern() code, but using drush's internal batch mode instead.
It's more maintenance, but maybe this is, in the long-run a cleaner implementation?
Would love some input.
Am posting my latest version of the drush.patterns.inc file with the yaml import removed, and a stock version of patterns_execute_pattern() pasted in and renamed to patterns_execute_pattern_drushbatch(), ready for rewriting using the drush batch API.
#17
You can set $mode to anything you want provided you implement patterns_execute_pattern_() function. In this case you could set the $mode to "drush" and implement patterns_execute_pattern_drush().
In the code I'm working on, I implemented patterns_execute_pattern_drush() in drush.patterns.inc and I directly call it instead of going through patterns_execute_pattern(). Then in patterns_execute_pattern_drush() I'm using drush batch API for execution. In any case this part doesn't pose any problem.
Real problem here is that certain actions depend on the result of the previous action being successfully executed. For example you can't create term for a vocabulary that doesn't exist. Those newly created items (e.g. vocabulary) are not available in static cache (because they were created after the caching happened) and next action fails assuming that such item doesn't exist. This happens in the case when static cache can't be reset. Progressive batch API was ideal solution because it would execute each action as a separate page request. Unfortunately progressive batch can't be used from within drush - see explanation here http://drupalcode.org/viewvc/drupal/contributions/modules/drush/includes....
Drush batch API is non-progressive and it works in most cases but fails for some actions. That's where I got so far. I still didn't look further into ways how to fix this but just wanted to give you an update so that you know in which direction things are heading.
#18
any chance we can get the file you're working on into cvs so I can see it and harmonize with what I'm working on?
we seem to be up against a brick wall with regard to batch support. can i assume that a workaround, using your latest code, would be to simply not put any dependent actions into the same patterns file? ie, if an action depends on the succesful outcome of another, the action should be processed as a separate drush pattern-enable call to a second file. I'm trying to push for a build-tool in some big drupal-based projects here that incorporates work with Patterns, and drush is essential for any buy-in.
#19
I didn't find the time to clean up my code and commit it so I'm attaching it here as it is. Please note that I took your code from #16 and just added my modifications to it which means this file contains the most current and up to date code and it should be used as a base for any further work.
Workaround you are suggesting should work provided the only issue we have is the one I described in #17. Until we do more testing we can't be sure if that's true or not. Besides that, that's still only a workaround and I think we should come up with a proper solution instead of relying on it.
#20
Is there documentation of which combinations of actions can bring about the error described in this issue? I'd like to see about resolving it, if possible. I'm happy with how drush patterns is running for my existing use cases, but I'd like to try it in other cases I haven't thought of.
#21
Hi again. Could you guys provide a use case that's not working? I'd love to be able to test this.