What I'd love is the ability to do a scan for coding standards across all of core.

But at the very least, it'd be rad if we could scan all the .inc files and stuff included in the module file.

Comments

douggreen’s picture

douggreen commented

I restricted the example.com/coder review to just the .module files because scanning lots of files at once can take a page a long time.

You can, of course, enable all of the core modules on example.com/admin/settings/coder and then example.com/coder will scan all of the .module files.

I could add an option to the settings page to scan additional file types (.inc), but let the user name others as well. I could also work at actually detecting the require and include files and read them, regardless of extension - however, many of these includes are dynamically generated, and attempting to figure them out will be problematic.

What I'd like to do is move some of these options from the settings page to the coder page. I'd like the selected reviews to be a selection at the top of every coder review page. And if we add your include option, that too should probably be an option at the top of every page.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

What I had pictured is another menu callback, like coder/all or coder/core... which would grep through all files in a defined extension list (.profile, .php, .module, .install) and check them over for coding standards violations. This would take a REALLY long time, though, and might run into PHP timeout issues. I'm thinking maybe a button you click on this page that starts the process, and then it does a little bit every cron run, similar to how the search indexing stuff works... with a big report at the end. This would require storage somewhere though. Hrm.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Add ability to scan all files in a module / core? » Add ability to scan all files in core?
Status: Active » Needs review

Here's a separate "coder_all.module" that does this: http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/webchick/code...

A couple observations:

1. This code:

  // get this once - list of the reviews to perform
  $reviews = array();
  $avail_reviews = _coder_reviews();
  $selected_reviews = variable_get('coder_reviews', _coder_default_reviews());
  foreach ($selected_reviews as $name => $checked) {
    if ($checked) {
      $reviews[$name] = $avail_reviews[$name];
    }
  }

...had to be copied/pasted from coder_page(). Probably makes sense to make this an API function... coder_active_reviews() ?

2. I originally had this as a sub-path of coder (coder/all), but the problem is that you're forced to load the original ?q=coder page in order to get there (assuming you use the navigation menu), which locks up for 30 seconds or so. Does it make sense to make the ?q=coder page just a "jump off" page with links/tabs to:
- Scan all active modules
- Scan all modules
- Scan all of core

?

3. I coded this as a secondary module because I wasn't sure a) how much overhead it would represent and b) how long it would take to do the scan. Turns out, neither of them are very bad. You've done a really good job of making it really easy to make custom scanners based on coder module. And in a clean core installation, anyway, the scan took pretty much as long as it does the module-only scan. Not sure what's up with that.

So. Should I re-roll this as a patch for coder module (and at the same time take care of 1 and 2), or keep it as a separate module?

douggreen’s picture

douggreen commented

I'd like to see better options, on the actual coder page. I like the idea of coder, coder/all, coder/active, coder/core. I think example.com/coder could really be the settings page that allows you to select the modules (all core, all active, select list) and choose the reviews.

douggreen’s picture

douggreen commented

I've made some significant UI improvements that:

(1) Puts a settings form at the top of all output pages
(2) Adds some AJAX/jquery to select modules
(3) Implements coder/all and coder/core
(4) Displays the results in collapsible/collapsed fieldsets. Include files are collapsed within the parent module. Only modules with warnings are uncollapsed.

These changes are currently in the HEAD branch. Once I get some confirmation from users that this is stable, I'll move it to the DRUPAL-5 branch and make a new release from there. These changes will not be ported to DRUPAL-4-7.

douggreen’s picture

douggreen commented
Status: Needs review » Active
douggreen’s picture

douggreen commented
Status: Active » Fixed

I should have set this to fixed. Please test with HEAD and let me know what you think :)

douggreen’s picture

douggreen commented

I've added an "experimental" cache option that dramatically improves performance when many files where most of them have not changed. As this is "experimental" still, the default is "off", so to test this you will need to enable it on the admin/settings/coder page.

douggreen’s picture

douggreen commented

I just ran it on a D5 core. If I do say so myself, this is really looking cool! The color coded warnings are nice. And the new experimental cache is great. I'm anxious to get your feedback and make a release of all the new stuff!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I know!! I've just been swamped with work stuff, and want to make sure I have time to look it over thoroughly. :( I'll try over my lunch break today. :D

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Fixed » Active

Actually, I don't see coder/all in HEAD. So opening this back up. Looks like the internals of do_coder_reviews have changed, so my poor module doesn't work anymore either. ;(

douggreen’s picture

douggreen commented

Actually, I used coder/active and coder/core. I was wrong when above when I said these features were in HEAD. They're actually in DRUPAL-5, but I think you know this from now from some of the other issues entered last night. Can we close this one back up?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Add ability to scan all files in core? » Add ability to scan all files?

No, because this feature request is about the ability to scan ALL files (every one. includes, themes, etc.) from the root of the Drupal install.

I've reworded the issue title so this is a bit more clear.

douggreen’s picture

douggreen commented

Ok, I think I get it. "core" is a special case. When selecting "core" you'd like to also scan the "includes" directory and the ".php" files in the root. I think you'd also like to scan everything in "themes" but I'm postponing that a little based on wanting to add support for themes.

I don't want to start at the root and just look for all php files (.theme, .engine, .inc, .php, .module) because it would end up traversing and getting more than we want (sites needs to be excluded, people put modules in the core modules directory, I think that at one time the 3rd party xmltemplate had to be downloaded and installed in the themes/engines directory).

So this is why it seems like a "special" case to me. When "core" do what I said above in the second line. I see the value in it, but would like to consider if it needs to be generalized any.

I'd like to add support for themes too. But it's an entirely different feature request. I don't think it's going to be too difficult, but does have some special cases, such as including more files. What do we do about the engine? And stuff like that. Maybe adding support for themes is the next "big" feature to add. Se should probably create a new issue for that.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

No, it should just scan _ALL_ PHP files in a given Drupal installation. So I can do a complete, site-wide check for code standards violations, security problems, etc. Has nothing to do with core vs. not core. If I downloaded just a version of core, then I could get it to scan core, but if I have a site from a client, I could scan the whole thing from there too.

If this is a "won't fix" because of the theme stuff, that's cool; I'll try and fix up coder_all module, since it works for my needs. :)

douggreen’s picture

douggreen commented

Sorry for the multi-part, numbered, indented response... maybe a little too formal, but it's what came out while I was writing/thinking about this.

What you're trying to do actually doesn't make sense to me.

  1. You must have a faster computer than me. Code reviewing an entire site, especially one that is location enabled, has just too many files and times out after 300 seconds.
  2. a site will frequently have non-Drupal php files, for example tinymce and importexportapi, which have little or no value to review
  3. I generally only want to code review modules I am responsible for:
    • for client sites, this is always in the CivicActions standard directory hierarchy:
      • sites/myclient/modules/custom
      • sites/myclient/modules/contrib-pending
      • sites/myclient/themes
  4. for contrib work, I have a separate Drupal install with the code review default settings enabled for only my modules.

Two possible action items:

  1. Because of #3a above, I can see the need to code review base on directory rather than module (or theme) and would consider a contrib module to distribute with coder if it was flexible enough to handle this. coder_all is a good start, but lacks some of the directory settings options I'd like to see.
  2. I see value in adding the Drupal files in the root and includes directory, but if this is not what you're asking for... I think it would be of value to anyone evaluating Drupal core before a release.

To fix coder_all:

  1. add #severity to $coder_args.
  2. do_coder_review now returns a $form instead of HTML output.
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Yes, I do have a fast computer and access to php.ini where I can raise the timeout. ;)

But sure, let's start with "I want to be able to code scan all of core (which includes the /includes files, installation profiles, .install files, and default themes) for coding standards violations." I can work with that. :) That would just involve changing the coder/core to scan all of these files,not just core modules.

Does that sound acceptable or not?

douggreen’s picture

douggreen commented

Select "core" now reviews the top level php and the includes directory. I'm leaving this open because I think I can add themes pretty easily, and when I do that, I want to be sure to get the core themes and engines.

douggreen’s picture

douggreen commented

coder/all (the new "all" tab) is "core" + every module + every theme. I believe that all that is missing is theme engine support. BTW, coder/core already does all core files + every core module + every core theme (as of the first theme support).

douggreen’s picture

douggreen commented

I added an access setting for coder/all (that you will need to grant permissions to), mainly so it can't be run on my demo site as it's a resource hog.

douggreen’s picture

douggreen commented
Status: Active » Fixed

Doing some issue housekeeping, ... I think this has been completed for some time

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)