I'm thinking it might be a good idea to convert the DMU to a Drush command that wraps around PHP CodeSniffer.
I've been running into some problems stemming from the fact that PHPCS is so low-level. It provides no context at all about the state of the module conversion. This is bad, because some conversion routines will depend on knowing state information about the 7.x module and what else has been converted. Not only that, but PHPCS has no way (as far as I know) to work with things at the file level -- the biggest example thus far being converting a .info to a .info.yml file, wholesale. PHPCS has a role to play in the DMU for sure, and it would be good to keep it compatible with Coder, but as I work on this more I'm becoming increasingly convinced that it simply doesn't cover all our needs.
Implementing the DMU as a Drush command would have these advantages:
- Everything the DMU depends on, including PHPCS itself, is installable via Composer.
- Installation would be drop-dead easy. I completely agree with xjm that the current install process will scare the kiddies away. 'drush module-upgrade', not so much. :)
- We'll be able to leverage Drush's command-line handling and context systems, wrapping easily around PHPCS's API.
Thoughts on this?
Comments
Comment #1
phenaproximaComment #2
webchickI think I'm for this. Having just gone through the installation instructions in README.md with fresh eyes, it'd be great if we could move "composer install + 2 manual steps" to just "composer install" or whatever. (That said, they actually seemed to work, which I was not expecting! :D)
Would have to be Drush 7 (master branch) though, for compatibility with Drupal 8. (Yes, that's terribly confusing :D)
Also note that there's some prior art of adding a Symfony Console wrapper in the "console" branch which came out of Mile23's work: #2194285-15: Make this project installable with Composer.
Pinged the rest of Team Upgrade Roboto to get their thoughts. :)
Comment #3
Wim LeersWhat do you mean by this?
In any case: +1 for simplifying the installation process!
Comment #4
webchickWe talked about this in IRC some. Basically, there are two main "categories" of 7 => 8 conversions:
1) "Convert some_thing() to someThing()" or whatever (basically a big find/replace). For those, PHPCS is basically perfect since that's what it's intending to do.
2) "Take this big array of procedural crap and convert it to one or more YAML files and/or one or more classes." For these types, executing the logic once per line of procedural crap makes absolutely no sense. You only need to do the check/conversion one time per file (or really, per module).
But PHPCS doesn't track enough 'state' information to make that second one easy. You'll see code in the InfoToYaml sniff for example that's like "die if this already ran once." So Adam's thinking is that by wrapping the PHPCS stuff in a class that can track that overall "state" we'd be better off.
Comment #5
Wim LeersThat's already much more understandable, thanks :)
If I understand it correctly, this is basically about being able to run DMU iteratively, while updating the module. To avoid expensive/useless work, we must track state. That still makes sense to me. But… where do you want to store state? And why/how does Drush help with that?
Comment #6
phenaproximaTo be honest, I'm not even sure we'll need to store state, beyond what DMU needs in-the-moment. This is mostly about sniffs and conversion routines being able to take a higher-level view of the module being worked on, precisely in order to avoid doing expensive/useless work.
So that's what I'm hoping we'll get out of using Drush -- it'll let us *above* PHPCS, directing the action and providing helpful contextual information to the sniffs, instead of being forced to stare at tokens and having no idea what else might be going on.
I hope that makes sense...I'm not good at explaining this stuff =|
Comment #7
Wim LeersNo worries, you're only getting started, once you're deeper in this matter, you'll be able to explain it much more clearly and explicitly :)
I'm personally fine with making it a drush command even if only for a simplified installation process.
I think you're basically saying that when we rely on PHPCS alone, that we can only ever execute token-parsing-and-reporting stuff, but we will also likely need some state tracking (probably in a
.dmu-state
file or something like that) to for example track.info
to.info.yml
conversion. PHPCS doesn't allow us to do that, so DMU needs to sit on top of PHPCS instead of within PHPCS, and rather than creating a new CLI tool, it's more logical to just use Drush instead.If that's accurate: +1 :)
Comment #8
webchickSo looking at the Drush integration in http://cgit.drupalcode.org/drupalmoduleupgrader/tree/?h=refactor so far, I guess two things:
a) Now our lovely project which attempted to "get off the island" has very, very Drupal-specific stuff like a .module file in it. ;) But I guess the benefit we gain from this is people can just do
drush dl drupalmoduleupgrader
and ta-da.b) However, we seem to also be duplicating code from the traits into the .drush.inc file, which seems rife for bugs to crop up from fixing something in one place and forgetting it in the other (specifically, both drupalmoduleupgrader.drush.inc and ReaderTrait.php have a function to parse the legacy info file format). This could be just an artifact of the fact that the code is in flux atm and I realize you're still working on it. But just raising it as a concern.
Comment #9
phenaproximaYeah, the legacy info parsing code is an artifact of my hacking around from yesterday. I'll kick it out of there once I've successfully wrangled PHPCS into fixing files when I tell it to. :)
Comment #10
phenaproximaThis is done and committed to the master branch.