This is a new Battleplan past #1745068: Coder Review Drupal 8.x Battleplan. Last year we gave Coder Review a facelift with regards to code cleanup, the ignore system, and PHP_CodeSniffer.

The following was discussed at the Prague Code Sprint with stella, jjchinquist, and forestmars, as well as discussed with a few random people. But before we embark on this too deeply, I would like more feedback.

This year I'd like to bring the Coder projects (Review, Upgrade, Drush, Grammer Parser, PHP CodeSniffer, etc) together in an integrated way. I propose that we re-organize the project into a Drush plugin that is Drupal version agnostic and keep the current project as only a UI module. Here are the specifics:

  • The Drush plugin will be a new Drupal module (not on github but hosted on Drupal) and we'll maintain all of the code in the master branch.
  • The Drush plugin will get a new upgrade command, yet to be designed, possibly based on Coder Upgrade or ideas from Coder Upgrade, or possibly based on https://github.com/fabpot/PHP-CS-Fixer and Twigify.
  • The Drush plugin will contain all of the review rules, and the UI will need to call the plugin as a library that returns results.
  • In 7.x, we'll start a 3.x branch of coder and this will be the UI module. It will still be called Coder Review in 7.x. 3.x Coder Review will use Drush Coder as a library and will get it's results from that library. Coder Upgrade will stay unchanged.
  • Coder Upgrade will be removed in 8.x, and Coder Review will just become Coder again. Coder Upgrade functionality will be in Coder Drush and the old module won't be necessary anymore. We're leaving it in the 7.x branch for people still using the 6.x to 7.x upgrades. Coder Review, which is just the UI portion, now will become just coder.module.
  • I hope that the Drush plugin upgrade command will alleviate the need or desire to continue with Coder Upgrade. (But if we mostly agree on this path and we go down this road, and someone disagrees, they are free to continue Coder Upgrade as a separate project.)

My plan is to write the framework and one or two upgrade examples, but we'll need the community to write the bulk of the reviews.

Some history

Coder (coder review) and Deadwood (coder upgrade) predate Drush. At the time I wrote Coder I was against re-writing code from the web interface (Apache) for security reasons. So we focused solely on identifying issues. A few years later Deadwood was created, and it chose to do something I was unwilling to do, rewrite code through the web interface. It also included a PHP command line though, that was fine to use without security concerns. We merged the two projects, but only in name. They never have played well together and have been mostly maintained independently.

Some Questions

  • Is the dependency on drush OK, if we fix the plugin/module issue and the gplib/grammer_parser dependency issue

Comments

geerlingguy’s picture

I would say a drush dependency is perfectly fine. I also think it wouldn't be a bad thing to make coder upgrades only available through the CLI, but I can understand that UX might trump legitimate security concerns there (I'm thinking mostly of beginner-to-intermediate level Drupal devs who have little to no familiarity with the command line—which is a lot of people).

Additionally, drush commands can be made to work through the UI in one way or another, like how the Migrate module does migrations 'in the background' with Drush, invoked through the UI.

jhodgdon’s picture

There are other modules that use Grammar Parser; notably, the API module (which does api.drupal.org).

So I kind of hope that the Grammar Parser library project and the Grammar Parser Lib module (which integrates with Libraries module) stay separate from coder* and drush*, and that both of these continue to have definite versions and not just a master branch. There have been incompatible changes in the past between Grammar Parser library branches, and the API module needs to have a definite version to work with. And I don't want to make the API module have a new dependency on Drush or Coder*.

I have no opinion on the Grammar Parser UI module, which API doesn't use.

douggreen’s picture

@jhodgdon, I was not aware that Grammer Parser was used elsewhere, but it makes sense that it is. We will not be touching Grammar Parser. At this point, I'm not even sure it will still be used, the answer of which depends on our research of PHP-CS-Fixer.

jhodgdon’s picture

It was, as you said there, a good idea to cross-post this to the Core Group on g.d.o, or I wouldn't have been aware of it. :)

douggreen’s picture

@geerlingy I really dislike letting the browser rewrite PHP files. The problem isn't that you can't do it. the problem is that you shouldn't do it because it is a security hole. I might be convinced if we rewrite files to a new directory, which I believe is what Coder Upgrade does today, and/or if we only rewrite disabled modules.

solotandem’s picture

All this talk of potential security issues is rather curious.

Why would code to be upgraded reside in a "live" code directory? And be enabled? (Unless you manually changed the version string Drupal should not allow it to be enabled anyway. If not enabled, how is it that it could be executed and pose a potential security hole?)

Why would someone ever write over "live" code? Why would that ever be a design choice? Yes, Coder Upgrade does NOT change "live" code. Even if you were to do so, the problem with upgrading code is that it is EXTREMELY difficult to make all the changes for various reasons like:
- you do not have the time to donate to write all the routines
- core API changes, regardless of the API change nodes used with D8, are not well documented nor completely documented by any means
- some changes are not cost effective to code, e.g. they affect such a small percentage of modules

douggreen’s picture

@solotandem, IRT security concerns with rewritable code, I can compromise and work harder to accept a solution. But I do believe any solution we derive should run through the security team, which is easy enough to do.

This might be our disconnect. I've always envisioned rewriting live code, because I always envisioned an iterative upgrade process... run the upgrade, have it tell you places that need manual attention, then run it again, and so on. But as I started this comment, I think we can figure something out, especially if that means involving more people in the process, such as @solotandem.

@solotandem and I had a conversation near the beginning of the D8 development cycle where I privately said I think that upgrades could become the most important aspect of coder, but that has never really become a reality. This issue is a re-commitment to do something for the Drupal Community for D8.

My proposal is to reboot the framework with a more modern architecture (drush, Symphony maybe), integrate the projects better, provide a few examples making the upgrade rules as simple as possible (but not simpler). The Drupal Community will have to step up to write most of the reviews. I don't expect fully functional code to come out of the upgrade process, and do expect @todo comments in the rewritten code.

douggreen’s picture

Despite the willingness to compromise on a UI, not a single person I talked to at DrupalCon Prague thought that requiring drush was bad. Contrary to that, many people thought that forcing drush would be good, in that it might get some developers to start using it, who had not yet started using it.

solotandem’s picture

Unlike reviews, I do not think it reasonable to expect upgrades to be run iteratively. To attempt this would add another level of complexity to the upgrade routines, namely to determine whether a given portion of the code has been or needs to be upgraded, or is partially upgraded. But maybe that is where integration with reviews would come in.

And, as far as "making the upgrade [rules] as simple as possible," good luck with that. (Anybody taken a look at the API changes from D7 to D8?) First off, let's refer to them as "routines" not "rules." As @douggreen and I have discussed, the more complicated API changes require upgrade routines that are several orders of magnitude more complicated than the corresponding review rules.

Unlike the push for automated tests, I do not expect the community to jump in and write upgrade routines. (I made an unsuccessful pitch for this a few years ago.) I do not expect anything to be done without funding to the developers involved. But talk of that gets us into the politics behind all this.

douggreen’s picture

I too am skeptical on easy, but it was a request from @webchick.

I agree with a lot of the above. I've found no one willing to write all of the upgrade routines. But we can either do nothing or do something. If we do nothing then the community has little chance to help. If we do something, as in setup/modernize the framework, then the community has a chance to pitch in, and Organizations wishing to upgrade will be more likely to sponsor writing upgrade routines.

I'm willing to work on the framework and a few sample rules. But I don't want to unilaterally decide the re-architecture. So I'll wait until we have more participants in the thread. Can we get 50 people to bikeshed this please?

webchick’s picture

I'd say time-box the discussion to a given date (say 2 weeks) and whoever speaks up before then, they get a say in the roadmap. :) 50 opinions aren't necessarily more valuable than 3-4 opinions from people with experience, IMO.

stella’s picture

+1 for time-boxing it.

I've no objections to having a dependency on Drush and can't see why anyone would. Also, another +1 for investigating PHP-CS-Fixer.

jjchinquist’s picture

Sorry for a long list :) I hope I cover it all.

  1. In general, we should not have to worry much about security. Coder is not intended to be used on a production site. The general permission level is higher than administer website
  2. Having iterations would be too complex. The module is in too early a stage for this. Make it simple, get us started.
  3. At Drupalcon I stated, drush dependency is a good thing. It is a solid tool that gets a lot of attention. Making a UI for it would also be fine as long as the correct permission settings are enforced.
  4. Keep the current file structure: rewrite the code located in /sites/default/files/coder/old/mymodulename --> /sites/default/files/coder/new/mymodulename. Do not make it more complicated and the security is ok.
  5. We could add the drush command to download the module from d.o. to /sites/default/files/coder/old/mymodulename automatically when the developer invokes the upgrade routine.
  6. Keep the current Grammar Parser Library dependency, but review it and "make it better". - this is my suggestion, I still want to look at it more in depth before we start.
  7. Twigify: I like the approach and example that Forest and the team he worked with used for it. There is one aspect I would adopt from Twigify, make the update routines a php class file and allow for consistent functions. Coder does not do this. Twigify needs to improve in two ways itself though:
    1. use drupal 7's and drupal 8's native file handler functions, not a 3rd party filehandler class. Both Twigify and Coder use custom filehandler set-ups which is not consistent.
    2. enforce strict folder and option input. Currently Twigify would be a security threat on live sites (see point 1) because I can choose live folders.
  8. Upgrade routines: I could forsee an initial stage for coder module where the original code is analyzed and a function in the appropriate file is declared with a note. It will allow us to get a scope of the work yet to be done on coder, and it gets the module maintainer started on the upgrade process.
webchick’s picture

Random thoughts:

- +1 for leveraging OPC (other peoples' code) to do these things, where it makes sense. It means we have less work to do ourselves, and we can help improve those projects for everyone when we find problems. Win-Win.

- I don't think the Drush dependency is a huge deal, personally, especially if it gains us the benefit of only having to do this major re-architecture once and have it work for the D9, D10, etc. versions. This is the very definition of a module developer-facing tool, and if module developers don't yet use Drush or know what it is, they will most certainly be thankful for the pointer. Plus, and really no offense to the people who worked on it, the Coder UI that I remember was not completely outstanding to the point that people would weep bitterly in the streets if it were to be removed (or, preferably, merely kicked to a later point in development when the Drush work was done).

- No one is going to write all of the upgrade routines without significant funding. 100% agreed, and we should look into seeing if/how we could fund something like a wholesale 7 => 8 upgrade path because that would be a HUGE win for D8.

- However, many of us would write 1-5 upgrade routines without funding, as we find things when porting our modules, as long as the framework for doing so was there, and there were some docs and examples on how to write an upgrade routine. I sat down to do this very thing when I ported Pants module from D7 to D8 for my DrupalCon Sydney session, but got completely blocked because the framework didn't exist. If it had, I'm sure I could've covered at least .info -> .info.yml and some of the other smaller changes. If even 1% of the people who are porting their modules right now wrote 1-2 routines, we could have a decent stock of them in fairly short order. (I realize that this did not happen with the D7 version of Coder Upgrade, and I'm not sure exactly why, though I suspect it was around the timing of when those learning resources became available? solotandem, do you have thoughts?)

- I do have a preference for easy-to-write upgrade routines, but I have a far greater preference for not duplicating work between Coder and Coder Upgrade if at all possible, and not creating a pile of code we need to maintain ourselves on an ongoing basis. So if leveraging Twigify/PHP-CS-Fix means the upgrade routines are a bit harder to write, but they only have to be written once, then +1.

solotandem’s picture

Regarding webchick's comments in #14:

I remain highly doubtful that "many of us would write 1-5 upgrade routines without funding." While there are some beginner level API changes, most are not. Writing beginner changes does not count for much in the overall as things in this category could often be accomplished by simple find-and-replace. But, like all of the changes, these would be facilitated by more thorough documentation of the API changes.

The process involved in writing an upgrade routine goes like this: research an API change (which can take several hours at minimum), create an example of code to change (this is basically the "write test first" approach), iteratively write an upgrade routine and test it on the sample code. This cycle often repeats as you reread the issue[s] that changed the API item, improve your understanding of the change, add edge cases to both the example code and the upgrade routine. Also involved in this is learning and improving the upgrade tools (or framework).

Thus, there is a huge time investment required to write one routine. And the "knowledge" gained is not likely to be "reusable" on other projects. So, again, I don't see any community involvement happening any time soon.

As far as "learning resources" there are a few pages on d.o. that were written (at your request) before anyone thought of getting involved with the D7 work. Also, like the example module, the advanced resources are the code in Coder Upgrade itself.

Everyone would agree it would be nice not to duplicate work between Coder Review and Coder Upgrade. However, this sentiment flies in the face of the evidence that identifying things to be changed is fundamentally different than making the change.

Code reuse and "not creating a pile of code we need to maintain ourselves" are also easily agreed upon considerations. Speaking for Coder Upgrade, my plan (already implemented in the code running on the upgrade site) is to separate the upgrade routines from the basic framework that invokes the routines. So, a module is created for each major version of Drupal core changes. The basic framework module runs in D7 and can run routines for any version of Drupal core (or contributed modules), but is probably not far from being Drupal version agnostic (like the Grammar Parser code). If so, then that basic code is very little "code we need to maintain ourselves." The upgrade routines are likely to remain very Drupal-specific; thus being our responsibility.

I do not anticipate great benefits from fitting all this into some "framework" especially if only for the sake of doing so. The basic engine is a big looping routine that invokes "alter" hooks. The routines are brute force PHP, not likely to lend themselves to some framework. But I am open to evidence to the contrary.

webchick’s picture

In terms of not duplicating work, I was referring more to if we used a tool like PHP-CS-FIxer, it looks like they have both FinderInterface and FixerInterface so I picture (in a total hand-waving way) the "finder" rules for Coder's scan to be using FinderInterface in order to say "Hey, you need to make this change and here's where you can read more about it," and then the Coder Upgrade "fixer" rules that are written could leverage those finders to say "And now I'm going to do that thing we just talked about for you." Since, as jjchinquist pointed out, detecting that something's not right and pointing you at the docs for it is imminently easier than writing the code to make the change directly, this would allow us to document the changes relatively quickly, and only fill in the fixer blanks on ones that someone felt strongly enough about to take the time to write an upgrade rule.

Rather than the current situation (iirc) where we have two different tools to assist with upgrades, and each of them knows about some things but not other things, depending on who got around to writing a detection/change routine for it (since both code bases have to do so), so as an end-user, neither tool provides the whole picture. (Note: we would not need to use PHP-CS-Fixer to benefit from Coder and Coder Upgrade sharing the same detection logic, it just at first glance seems to have the proper architecture for it already, so worth investigating.)

So maybe it's more apt to say that we would write 1-5 "detection" routines without funding (which seems fairly realistic, since we don't need that kind of rigor on simple detection), though also perhaps a handful of "upgrade" routines if they saved more time in aggregate than it takes to write the code once. In the case of renaming "function_foo" to "function_bar" that's probably not the case, and it'd only be a detection routine, and the fixer routine just a "@todo". But in the case of things like turning hook_menu() into a series of controllers? I could see dumping some real time into that, just as duellj et al dumped real time into the DBTNG converter in Coder Upgrade 7.x. It saves tons of time on every single module port.

Regarding the lack of community uptake in helping with Coder Upgrade in 6 => 7, I definitely agree that happened. But could it have been less about laziness, and more about the lack of visibility around this being an avenue to contribute, and a critical tool in the upgrade process? And if so, is there a way we could combat this with e.g. a prominent landing page when we launch D8 that mentions this as a key developer resource? Multiple Drupal Planet posts about it? I'm not sure. But I don't think just because we didn't see those results in the 6 => 7 port, we should throw up our hands and give up on the community helping with this in 7 => 8. If anything, because of all the changes, I'd say there's a much higher incentive for people to contribute this time around, if we can reach right people with the time/inclination to help.

jhodgdon’s picture

A few quick thoughts:
- Waiting until Drupal 8 releases to say that Coder is a great way to contribute is not necessary. :) I'd say when the platform is stable and we have a few examples done would be the time to start a social media blitz (the Core group on g.d.o, planet, and/or twitter). This is not really tied to the 8 release cycle.
- I like webchick's outline idea of having an interface for Review and an interface for Upgrade that the same class could implement, whether or not we use that particular 3rd-party tool.
- (Third-party tools that do pretty much what we need) ++

And one bigger thought:

Simpletest got elevated in status as a tool when we put it in Core in 7. Examples is (possibly) poised to do the same in 8, and Views ... well Views already had an elevated status. So... Would it be possible (eventually) to put at least the Coder framework in Core -- at this point, we'd have to think of D9, I'd guess, but even the thought that the Coder framework could be so important that it's being considered for Core would I think be a great boost in visibility. Maybe we could make it an official Core Initiative, even right now, with the understanding that it's such a key tool and that we want to aim for Core in D9?

I guess maybe putting it in Core wouldn't be viable if it's all Drush dependent, but I still think making it an Official Initiative might be a Good Thing.

solotandem’s picture

Based on the evidence, more non-core tools in core is not a good idea. For example, putting simpletest in core only slowed and/or stymied the development of the testing system. Getting into core brings the pace of development to a halt and ties the hands of developers to the labyrinth that is core development.

jjchinquist’s picture

I agree with solotandem about Coder in Core. This is not a true "core" item, coder does nothing to help the system in a production environment setting and is not even intended to be installed in a producton environment. Lets keep it simple.

If there were a way to create pluggable, small and simple upgrade routines, I would be all for it. Webchick, do you have a proposal how that could be acheived?

webchick’s picture

So, I'd say sufficient time has been given for folks to chime in here. What are the next steps?

jjchinquist’s picture

So, the next 2 steps are to get the 8.x-3.x up in a skeleton framework, making the neccessary changes to the structure first. After that getting the workflow functioning and finally the rewrite commands and rules.

jjchinquist’s picture

jjchinquist’s picture

Forest set up the following sandbox at https://drupal.org/sandbox/forest/2106549. We will do a bit of development and testing there for now. The idea is to move it to the coder module at some point.

jjchinquist’s picture

And we are looking at the following too: https://groups.drupal.org/node/379823

webchick’s picture

Awesome! But unless I misread, https://drupal.org/sandbox/forest/2106549 looks like it's just re-implementing Coder Upgrade in different Drupal-specific code, and https://groups.drupal.org/node/379823 is a re-implementation of https://drupal.org/project/module_builder, not actually related to upgrading code.

The battleplan here proposes leveraging non-Drupal code for the task of upgrading from one version to another. Can you explain how either of those projects fit in?

webchick’s picture

Unfortunately, I don't think that https://github.com/fabpot/PHP-CS-Fixer/ is going to work for our needs, since it's pretty heavily focused around specific checks for PSR-1/PSR-2. Here's an overview though, since I had a chance to dive into it today.

Basically, php-cs-fixer just has one command, "fix." You basically run it like this:

php-cs-fixer fix /path/to/project

It will automatically re-format the code within that path according to various standards. By default, PSR-0, PSR-1, and PSR-2.

You give it a configuration, for example https://github.com/fabpot/PHP-CS-Fixer/blob/master/Symfony/CS/Config/Mag... which lists the name/description of the rules. So we could make a Drupal specific one.

In the constructor of the Config, you give it reference to a "Finder" which basically describes the folder layout of your project, for example https://github.com/fabpot/PHP-CS-Fixer/blob/master/Symfony/CS/Finder/Mag.... Here you can tell it to ignore certain folders, to treat weird extensions as PHP, etc.

Then a series of "Fixers" are ran, which you can see a list of at https://github.com/fabpot/PHP-CS-Fixer/tree/master/Symfony/CS/Fixer. Here's a simple example: https://github.com/fabpot/PHP-CS-Fixer/blob/master/Symfony/CS/Fixer/Shor... Each one implements a "fix()" method which basically just runs preg_replace() code over the source. You can set the "level" (defaults are PSR0, PSR1, PSR2, and ALL), as well as a "priority" (like a weight, so one fixer can run before/after another), and so on.

In order to leverage this code for Coder Upgrade, we'd basically have to blitz all of the current Fixers (since Drupal doesn't follow PSR1/2) and add a bunch of our own custom Fixers which would never be accepted upstream. And then finally, preg_replace() is a lot less elegant than the reflection stuff that Grammar Parser uses. So it looks like the wrong tool for the job, unfortunately.

I still though cling to this idea that it would be really amazing if we could leverage the same code for flagging problems in both Coder / Coder Upgrade, but then only implement "now fix it" logic for certain rules in CU, and could fill them in as we get a chance / as volunteers step up. I might poke around our Code Sniffer stuff to see how that works and if it could be re-used in that way.

webchick’s picture

Although, pdrakeweb did make a pretty decent stab at Drupal coding standards rules for PHP CS Fixer over here: https://github.com/fabpot/PHP-CS-Fixer/pull/136

Also, having re-read the issue summary now after many months, I see that I'm completely barking up the wrong tree anyway, since it says in black and white that the idea is to separate Coder from Coder Upgrade and boot the latter out of the project in 8.x. Dang. :(

It doesn't look like https://drupal.org/project/coder_drush or https://drupal.org/project/coder_upgrade exist yet, though.

In mid-February what I'm hoping to do is spend 4 solid days w/ a few other Acquia folks on a basic proof of concept Coder Upgrade for 7.x => 8.x. I was really hoping to do this in such a way that doesn't require two different sets of maintainers creating and maintaining two different sets of "sniffers" for upgrade issues though. :(

webchick’s picture

Also, just in case it's a total non-starter I'm doing this on Github for now, but https://github.com/webchickenator/drupal7to8-codesniffer is kinda what I'm referring to.

Here I've created a CodeSniffer standard "Drupal7to8" and in it, we can write individual sniffs for all the major items at https://drupal.org/list-changes. I threw a very simple one up as a PoC to start: https://github.com/webchickenator/drupal7to8-codesniffer/blob/master/Dru... that warns you if you have a .info file that it should be .info.yml now, and points you off to the change notice in question.

Each of these are (hopefully) only a few lines of code, doable by almost any PHP developer, once some decent docs are written (like PHPCS's class naming convention totally threw me for a loop). The knowledge requited to write these upgrade sniffs would be the same to write Sniffer routines for any other PHP project.

If this is acceptable, then I believe this would eliminate the need for maintaining both coder_review and coder_sniffer, in favour of just one set of upgrade sniffs that both projects can use (sniffer), and a UI that spits them out (review).

Then, that only leaves us with Coder Upgrade (the "actually make those changes to the code") part. For this, I'm thinking of creating a generic "SnifferFixer" project (so akin to php-cs-fixer, but without needlessly duplicating the coding standards rules in two places) that takes PHPCS errors as input and calls "fix()" methods (or whatever) somewhere that actually attempts to solve the problems it finds. (Maybe this is a separate PHP_CS_Fixer interface which these sniffs could optionally implement if they're easy to fix?) There will obviously be a lot less of those fix() methods than there are process() methods, but they could filter in gradually as someone manually fixes them often enough that the desire to automate that repetitive task outweighs their desire to be lazy. :D The main thing is making sure the changes that need to be done are identified quickly, and that those rules are easy enough to crowd-source.

Anyway, what do you all think of my hopes and dreams? :P

webchick’s picture

Ok, I'm just about done spamming this issue, but just wanted to point out that per my last point, there's a phpcs-fixer branch of PHP_CodeSniffer on Github that does exactly this. :D Docs: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Fixing-Errors-Automati...

Awesome-tastic. I think I'll continue down this road and see how far it gets us then.

webchick’s picture

Last one for awhile, promise...

Here's an example of a normal "flag an error" converted to "fix an error" using the version of PHPCS in the phpcs-fixer branch:

https://github.com/webchickenator/drupal7to8-codesniffer/commit/a80315cf...

Basically it works like this. Reminder: process() runs once per line in a file.

  function process() {
  ...
    $tokens = $phpcsFile->getTokens();
    if (strstr($tokens[$stackPtr]['content'], '7.x')) {
      $fix = $phpcsFile->addFixableError('Upgrade core version to "8.x" in .info.yml file: https://drupal.org/node/1935708', $stackPtr, 'CoreTo8x');
      if ($fix === true && $phpcsFile->fixer->enabled === true) {
        $phpcsFile->fixer->replaceToken($stackPtr, "core: 8.x\n");
      }
  }

In the branch, there's a new addFixableError() method, which flags the error as something that can be fixed with the phpcbf script. If phpcbf is running, then phpcsFile's "fixer" property will be set to enabled. And here you can call the replaceToken() method on it to replace the current line with a different line.

I still need to look into it more and see if there's a way to do fancier things; e.g. on hook_menu() we don't actually want to replace individual lines but rather delete the whole thing and replace it with a new YAML file + a new class file + whatever. But... promising!

webchick’s picture

In 10 days, I'll have at least 3 Very Smart Drupal People™ to help create a proof-of-concept of Coder Upgrade for D8. It'd be awesome for anyone at all to give me even a vague "yes, that looks like an ok direction to me" or "STOP ABORT ETC" on #30 before then. :)

mgifford’s picture

I can't give you a nod either way on #30 @webchick, sorry.

Can #1745068: Coder Review Drupal 8.x Battleplan be marked as a duplicate of this issue?

Would be great if it could include a security review #2160605: Integration with phpcs-security-audit

Would also be great to link this into d.o projects #1623462: Tie in drupal.org with Coder Upgrade after D8 released

klausi’s picture

I agree that the phpcs-fixer branch looks like the most promising solution, so I think webchick is on the right track. I have not worked with that yet, so I can't tell if it will suffice for all code rewriting needs we might have.

jjchinquist’s picture

Thanks Webchick and klausi for getting in on this discussion. Can you commit to a sandbox a skeleton of what you are planning?

And can you elaborate on who will be joining you to work on coder in 10 days or at which event?

webchick’s picture

Yay! Thanks, klausi!

The sandbox is here for now, https://github.com/webchickenator/drupal7to8-codesniffer/ because given the issue summary I'm not sure where to post it on d.o. http://drupal.org/project/drush_coder doesn't exist yet, and the issue summary also says that Coder Upgrade is getting the boot from the rest of Coder in D8. Now that the coding standards checks *and* the upgrade checks are using the same basic framework for their rule definition, and that the code I'm writing will also be command-line driven rather than through the UI, I'm not sure how/if this changes things.

Feb 10 - 14 is Acquia Build Week where all of the engineers from Acquia fly in together to hang out and do "hackathons" (basically code sprints + demo) for the last 4 days that week. This is my hackathon project. I know at least Gábor Hojtsy and Lisa Backer (from Mollom) will be helping, but I'm also hoping to recruit a few more "on the ground." :) Obviously, others would be welcome to sprint remotely, as well.

stella’s picture

I have to apologise for not working on this more, or at all, but have been kinda unwell. I also can't promise that I'll be able to work on it in the near future due to kid #2 due any day now, but hopefully I will be able to help test / review in another month or so all going well.

jjchinquist’s picture

Several times this past year I looked at re-coding Coder kept having the large issues of where to start, how exactly to form the code, which tools to use, etc. So I am grateful to anyone who can give it direction and get the main functionality completed. Please get a list of issues/tasks together and I can help out a bit.

webchick’s picture

We've moved development to a Drupal.org sandbox here: https://drupal.org/project/issues/2193593

Feel free to add an issue and add yourself to the meta issue at https://drupal.org/node/2193641 :)

webchick’s picture

So we're definitely not close to done yet, but https://drupal.org/project/issues/2193593 is making decent progress. It covers:

- Renaming / reformatting the .info file
- variable_* to CMI conversion
- Basic hook_block/hook_menu conversion
- Some smaller changes like cache_*, language_*, etc.

Do we:

1) Rename the sandbox to Coder Upgrade, per the issue summary (and possibly rename it to something more intuitive that people could actually find, e.g. "Module Upgrader" :P)
2) Commit this to Coder as a sub-sniff of coder_sniffer (though that makes composer/packagist stuff awkward)
3) Other?

If 1, we can go ahead and proceed and then I'll stop invading your poor issue. :)
If 2, some subset of xjm, webchick, Wim Leers, Gábor Hojtsy, japerry, estha need commit access to Coder module. Trust me, you most likely do not want to take on maintenance of this code. :D

klausi’s picture

+1 for continuing coder upgrade (or module upgrader or whatever you wish as name) as separate project. We can always reconsider that later.

The only thing we probably want right now is a helpful link and explanation on the coder project page, so that people that come looking for coder upgrade can find you :-)

webchick’s picture

Ok great. We are now at https://drupal.org/project/drupalmoduleupgrader. :) See you there!

klausi’s picture

I released Coder 8.x-2.0-alpha1 some time ago, mainly focusing on updating the Coder Sniffer Rules to the phpcs-fixer branch of PHP CoderSniffer. coder_sniffer can be used independently of Drupal and drush, so I also released it as project on packagist for easy install with Composer: https://packagist.org/packages/drupal/coder

Good news: you can now run

./vendor/bin/phpcbf --no-patch --standard=./coder_sniffer/Drupal /path/to/mymodule.module

and Coder Sniffer will fix some coding standard errors for you in mymodule.module! This is not complete for all custom sniffs we have, but at least for the inherited stuff from PHP CodeSniffer.

And we have PHPUnit tests now, yay! Run them with

./vendor/bin/phpunit

coder_upgrade has been removed from 8.x-2.x. Is anyone interested in porting the coder_review regex rules or should we just drop them? (Coder Sniffer catches almost all of those)

Should we close this issue now and open new ones for the remaining coder_review and drush integration stuff?

webchick’s picture

Awesome, that rocks. And sure, sounds good to me!

douggreen’s picture

@klausi @webchick Do you want to have a skype chat about the future. I think that my original vision has been surpassed by all your great work. I'm happy to contribute some. But I really don't know where to start now.

I hope/think that the original rules engine is now obsolete. And we should rely on php codesniffer going forward.

webchick’s picture

Hey there! :) Doug, Klaus, and I had a chance to sync up today about things, here's a summary of the talk (please correct if I got anything wrong):

- While the initial reasons for Doug proposing moving Coder Upgrade out of Coder were slightly different, in the end we agreed that it's really best as a separate project. Both for better visibility (it's now a top-level project, actually named what it does :)) but also because a successful maintainership model for Drupal Module Upgrader is likely going to be dozens of people each working on different parts; similar to Devel in that respect. This is in contrast to Coder Review, which is more tightly controlled since it effectively dictates coding standards for the Drupal community. Therefore, Doug is going to remove the Coder Upgrade folder from the 8.x branch, and point people to DMU instead.

- However, there's still a lot of value in both projects sharing the same "finder" logic, using PHPCodeSniffer to point out things that are wrong. Both projects then benefit from things like IDE autocompletion, and there's opportunity to abstract utility methods out that either standard could use. Since finder rules are about 50,000 times easier to write than "fixer" logic, it makes sense for DMU to focus on as many of these, in as high-impact places as possible. Klaus is planning to look into the possibility of a "Utility" standard, both for DMU and Coder Review, but also for the Drupal Practice sniffer.

- DMU might eventually move to a different "fixer" logic from PHPCS, due to the "rip out entire chunks of code entirely and replace with something completely new" nature of most D7=>D8 code changes, which don't lend themselves well to the PHP Tokenization approach (or at least not that we've seen so far). We're evaluating pharborist as a possible library to leverage for fixing logic. Either way is fine though, since that code would by nature be specific to DMU anyway.

- Doug is worried that more work hasn't been done on DMU yet, given how critical it is to Drupal 8's success. In the past, Coder Upgrade rules have been "chasing head" for months or even a year+ prior to a Drupal core release. Doug has been out of D8 development so doesn't feel he can fill this niche, especially while paying the bills. Klaus is "scratching his itch" with Coder Review and doesn't have such an itch for DMU. Angie is interested in doing onboarding/promotion around DMU but can't really play a lead architect/"firehose of code" role. We all agreed it'd be fantastic to try and get some "new blood" interested in this problem space and help them be successful. Angie is going to write up some documentation for DMU to help with this (both a general "highest impact rules" roadmap + some "getting started for rule writer" docs), and also promote the project at DrupalCon Amsterdam if her core conversation is accepted, to help with this.

- Another open item is the UI of Coder Review in the 8.x branch. This could use a revamp, and ideally would be able to present PHPCS results from any of these standards. Unknown who can take this on.

We agreed to sync up again sometime in ~mid-August, after D8 is in beta ($diety willing) and DMU becomes a more pressing need. In the meantime, we're planning to hang out in #drupal-codereview to keep in sync, so if anyone reading this has an interest in helping with any of these, please feel free to join as well!

I think that was everything. :) Go team!

webchick’s picture

One last follow-up to DMU since there are people here who presumably care about that. I got approval from Dries to put out an internship for someone to focus on it for the summer. It's late in the year (for North America) so not sure we'll get any hits, but here it is: https://groups.drupal.org/node/430423

klausi’s picture

Status: Active » Fixed

I think we can close this now with the following outcome:

Upgrading modules is now https://www.drupal.org/project/drupalmoduleupgrader
Coder 8.x-2.x will just be Coder Sniffer, a set of tools that can be run from the command line.
Coder Review has been removed, so Coder is not a Drupal module anymore. Any volunteers welcome that want to build a UI for Drupal 8!

I'm thinking about moving DrupalPractice https://www.drupal.org/project/drupalpractice into Coder, but I will open a separate issue for that.

klausi’s picture

Status: Fixed » Closed (fixed)

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