Jens Reinemuth on the development list suggested:

>>
What about e.g. a simple cronjob running preivious to the packaging,
which simply lets a php-cli parsing the files... so such errors would
not occur in packages.

>>

I'm not familiar with the d.o packaging scripts, but this seems like it'd be a useful addition, would probably need some error-reporting to the maintainer(s) to indicated why the -dev release wasn't created, but opening this to record it as an idea at least. I guess this could move over to the testing framework at some point in the future, but probably worth a discussion on how best to do it.

Did a couple of quick searches of the issue queue, and couldn't find a duplicate of this anywhere.

Comments

sdboyer’s picture

The concern I'd have with this is the same one that drumm and I have discussed re using Reflection for api.module: you're parsing in unknown code, which means the operation needs to be sandboxed somehow. Otherwise people could execute arbitrary code just by doing it in global scope. I have a theoretical fix for that, but not one I've yet found time to work on.

greggles’s picture

We can just do php -l filename.module without security concerns, right?

I have a little script in my "bin" directory called phpchecker:

#!/bin/bash
find ./ -type f -exec php -l {} \;

I imagine something similar that runs specifically on .php, .module and .inc files would be good. We also have http://ftp.drupal.org/files/projects/Amare.status for every module/theme. We could better integrate this check into that existing .status file system *and* integrate those .status files into the project page.

I agree about the concern for simpletesting random patches to all of contrib, but not a php syntax check.

sdboyer’s picture

yup. php -l ftw; the concerns about parsing in code re: Reflection do not apply.

/me cans it.

kbahey’s picture

Yes, php -l does wonders. If you use vim, you can have a command called :make that would run it and position the cursor at the error if the parsing fails.

We have to be able to parse .install, .inc and .module. But are there other extensions that we should parse? Perhaps search for <?php in all files of a project and run php -l on those that match.

sdboyer’s picture

If you use vim, you can have a command called :make that would run it and position the cursor at the error if the parsing fails.

+1 to my .vimrc

Perhaps search for <?php in all files of a project and run php -l on those that match.

+1 to that. Actually, the grep for php tags might not even be necessary, as php -l seems to do it internally; it reports no errors on non-php files (even binaries), and does so very quickly. So, is there any potential harm in just always running php -l?

greggles’s picture

For the few modules that ship with patch files php -l will report a false positive syntax error on the patches.

.module
.install
.inc
.php

Anything else reasonable?

merlinofchaos’s picture

Some modules use .theme for theme functions.

And there's the very very rare .engine file

John Morahan’s picture

.test
Perhaps modules with tests are less likely to have blatant syntax errors, but who knows.

catch’s picture

.module
.install
.inc
.php
.theme
.engine
.test

seems like a good list.

killes@www.drop.org’s picture

I think we should add such a check to the xcvs pre-commit hook.

dww’s picture

FYI: there's no hidden magic on d.o for either of these requests. All the relevant code is publicly available for anyone who wants to patch either script:

Packaging:
contributions/modules/project/release/package-release-nodes.php

CVS pre-commit hook:
contributions/modules/cvslog/xcvs/xcvs-commitinfo.php

Enjoy,
-Derek

aclight’s picture

I'm not sure that either of these suggestions is a good idea, but specifically checking for parse in the pre-commit hook seems like an especially bad idea. CVS commits are not atomic, so it's possible that if a user was committing changes to several files, and only one of those files caused a parse error, that the other changes *would* get committed. I think that this situation could be both confusing to the majority of our CVS users who don't really understand how CVS works, and also could be counterproductive because lacking some intended changes could lead to even more problematic behavior.

If we were using SVN or another RCS that was atomic, we could block the entire commit if there was a syntax error in any of the changed files. But that won't be the case for some amount of time.

dww’s picture

aclight brings up some good points about the problems of doing this at commit time. Here are some problems doing it at packaging time:

By the time we're trying to package, the faulty code has already been tagged and a release node exists (without a tarball and unpublished, but it's still there). So, even if we prevented this broken tarball from being created, we'd still have this unpublished, broken release node which would prevent the maintainer from moving their tag to the fixed version. So, either we'd have to change that stuff, they'd have to "fix" the release by making a new tag, or it'd require project admin intervention each time someone tagged a bogus release.

Therefore, perhaps we should do the syntax checking at tagging time. That is (more or less) atomic, and, it'd prevent the release tag from being created early enough for the maintainer to actually fix the problem themselves. Plus, it'd be immediate feedback that something is wrong, since they wouldn't get as far as the packaging script trying to build the tarball, having to come back to the release node and see the error message, etc. Anyone interested in this approach should check out contributions/modules/cvslog/xcvs/xcvs-taginfo.php.

aclight’s picture

I'm not in favor of doing this at all, because I think that this adds even more complexity to a system which is misunderstood by many, and I would anticipate that this will bring up even more problems for the admins to handle. I wonder whether a good middle ground might be some kind of warning on the release node for releases which cause syntax errors when the packaging script runs.

dww's idea about checking on tagging is good, except that it seems that a lot of the maintainers use CVS GUIs such as Eclipse, TortiseCVS, etc, some of which do not display error messages to the user or don't make the source of errors so obvious. So, for those maintainers using such GUIs, if there are syntax errors when they try to create a tag they might not realize why they can't create a tag.

dww’s picture

Yup, it's true, there's no end to the usability problems that CVS GUIs create for our users. :(

There's plumbing in place to have warnings/errors from the packaging script appear on release nodes. So, if we want that, it's not hard. See package-release-nodes.php (linked above) and search for the {project_release_package_errors} table. There's no API to use it, you just insert a row for the release nid you're working on.

eliza411’s picture

Status: Active » Closed (cannot reproduce)

Closing old issues. Please re-open if necessary.