Namedspaced code throws and catches "Exceptions" which don't exist (causing the DB settings form validation to break on install) [#1506630]

Since the PSR-0 conversion, there is now a lot of code in Drupal that is namespaced, but does not properly do a "use Exception" before trying to throw or catch instances of the Exception class.

The result is that PHP doesn't recognize what an "Exception" is. The most obvious symptom (and the reason I came across this) is that when installing Drupal 8, if you enter incorrect database connection information on the database settings form, you no longer get a nice form validation error, but instead it cuts off the install and prints an exception to the screen.

The attached patch fixes that, as well as some other cases I found looking through the codebase. (I haven't tracked down what issues the others cause, but I'm marking this critical because in general, any code that tries to "throw new Exception" when the Exception class is unknown will lead to a fatal error, and we have some of those in the codebase.)

Comment	File	Size	Author
#11	Exceptions_patched_first_pass.txt	40.37 KB	Alan Evans
#9	Exceptions_patched.txt	39.65 KB	Alan Evans
#7	Exception_files.txt	2.88 KB	Alan Evans
#7	Exception_matches.txt	38.88 KB	Alan Evans
#6	namespace-exception-1506630-6.patch	3.94 KB	David_Rothstein
#6
	namespace-exception.patch	2.07 KB	David_Rothstein

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

BMDan CreditAttribution: BMDan commented 31 March 2012 at 13:47

Comment #2

lotyrin CreditAttribution: lotyrin commented 1 April 2012 at 00:56

Status:

Needs review

» Reviewed & tested by the community

This all seems to look good and check out.

My only question is are these all of the places we'll need to do this?

Comment #3

chx CreditAttribution: chx commented 1 April 2012 at 02:36

Status:

Reviewed & tested by the community

» Needs work

Thanks for working on this. There are many solutions to this problem and I think the real resolution is not to throw Exceptions but specific classes as most of the codebase already does.

Comment #4

David_Rothstein CreditAttribution: David_Rothstein commented 2 April 2012 at 03:13

Three of the five files in the patch (the ones that are part of the database API) only catch Exceptions, not throw them. So we couldn't change that without a careful audit of what types of exceptions might actually come their way... Not really part of this issue, I think (especially since we'd still likely need to "use" one or more of them).

Of the other two, ArchiveTar.php throws exceptions but looks like it's external code written by someone else and included in Drupal (although it's already been changed in places for other Drupal-specific reasons, like drupal_unlink()). Not sure if we want to change that.

Changing Zip.php to use something more specific probably does make sense, and it looks like the code already even has a @todo for that!

Comment #5

aspilicious CreditAttribution: aspilicious commented 15 April 2012 at 20:21

So whats the plan for this?
I don't think we should change archivetar some more...

Comment #6

David_Rothstein CreditAttribution: David_Rothstein commented 17 April 2012 at 03:27

Status:

Needs work

» Needs review

File	Size
namespace-exception-1506630-6.patch	3.94 KB

Here's a new patch (untested) which adds a new exception class to Zip.php.

It also fixes what appears to be another remaining instance of this problem (in Drupal/Core/Updater/UpdaterException.php).

Comment #7

Alan Evans CreditAttribution: Alan Evans commented 18 April 2012 at 09:37

File	Size
Exception_matches.txt	38.88 KB
Exception_files.txt	2.88 KB

Attempting to do an audit of whether we have covered here all files that could be affected ... What I'm searching for is all files that contain the word "namespace" at the beginning of a line, and the subset of those that also contain the word "Exception" case-sensitive. there should be a number of irrelevant matches included here. One attached file is all the file names, the other is the matches themselves. I'm intending to get through these myself, but attaching here for the record.

I've excluded Symfony partially on the assumption that they'd probably do that right thing with namespaces, and also, glancing through, they do seem to fully qualify their Exception classes.

I'm wondering how we could possibly add tests for this that would expose the right issue. It doesn't seem feasible that there would ever be a test that could cover future files that might forget to use Exception.

@David_Rothstein - good to see you :) Are you sure "use" is better than fully-qualifying the \Exception in this case? I guess the answer is in typically how many times Exception is used within a file, but the fully-qualified name does only add one character in any case (but could be many lines in a given file). We just need to be aware though that whatever we do here might become convention, and I think we'd want to avoid importing a tonne of individual global classes into namespaces.

Comment #8

Alan Evans CreditAttribution: Alan Evans commented 18 April 2012 at 09:50

(Also verified that the latest patch does fix the install process DB settings validation as intended, btw)

Comment #9

Alan Evans CreditAttribution: Alan Evans commented 18 April 2012 at 10:51

File	Size
Exceptions_patched.txt	39.65 KB

Attaching list of Exceptions found within the patched codebase (might be more helpful than the previous list)

Comment #10

Alan Evans CreditAttribution: Alan Evans commented 18 April 2012 at 11:53

There are a couple of things I'm noticing while going through the text files that might also be good to get addressed here, namely inconsistencies:

Some cases use the fully qualified exception name, some do not eg. ./core/lib/Drupal/Core/Config/SignedFileStorage.php:40: throw new \Exception('Signature file is invalid.');
Some cases are within the same namespace, but appear to import the class anyway (even though the namespace should be the same and the import shouldn't be needed)
Some @throws comments use the fully-qualified name, some do not

There aren't a huge number of these, so I think it would be good to include them when dealing with this issue. We still need to think which is preferred for "Exception", using the fully-qualified name, or import.

I'd be open to suggestions though whether we want to use this issue for the functional part and open another, less critical, for the cleanup, but the cleanup seems small enough at the moment that we could consider including it here.

Comment #11

Alan Evans CreditAttribution: Alan Evans commented 18 April 2012 at 13:27

File	Size
Exceptions_patched_first_pass.txt	40.37 KB

I've been through my grep results and in summary I think David's attached patch would fix the immediate critical, but I also found enough existing inconsistencies that make me think it would be worth trying to get this patch through first (David's fix for the critical issue) and then deal with the cleanup points in a separate issue, as I'm sure those will cause debate and delays that should not hold up resolution of this critical.

I'll post a more thorough summary later. Attaching my notes from reading through the grep. It might be worth resolving the policy question of whether to import classes into a namespace or fully-qualify them.

Comment #12

Alan Evans CreditAttribution: Alan Evans commented 18 April 2012 at 22:01

#6 patch looks good from review, solves this critical, and I have not been able to find other instances in the codebase that could trigger this fatal which are not covered by the patch.

I think we could take a shot at adding tests just for the initial issue mentioned here: test for absence of validation error messages if you put in nonsense on the database connection form. Aside from that I still can't see how to add any decent generic test that would catch the absence of namespace imports from anywhere in code. We need to rely to a certain extent on other tests getting tripped if this happens elsewhere in code.

Comment #13

David_Rothstein CreditAttribution: David_Rothstein commented 19 April 2012 at 03:50

Hi Alan! Thanks for working on this.

Based on http://drupal.org/node/1353118 I think "use Exception" at the top of the namespaced file (rather than "\Exception" within the file) is the standard, so the places in core that don't do that currently should probably be fixed. (Some parts of those coding standards are still under discussion at #1323082: Establish standards for namespace usage [no patch], but I think the discussion doesn't touch on that part of them so far.)

As for tests, it would be interesting to see if this is testable. I don't think there's any way to do it with the installer directly, but I wonder if it would be possible to grab that database connection form and render it outside of the installer and try to submit it from there (especially since for this test we don't need/want a successful submit anyway, rather just need to hit the form validation). Alternatively, we could try bypassing the form entirely and just write some test code that calls the database API's install tasks directly?

Comment #14

Alan Evans CreditAttribution: Alan Evans commented 20 April 2012 at 21:04

Looks like that will work - pull the database settings form into a new page in a test module, put non-empty garbage into username and password fields, and without the above patch it blows up.

Comment #15

Alan Evans CreditAttribution: Alan Evans commented 21 April 2012 at 15:52

Close to giving up on the test - the main problem is that the form validates using the existing test site's database connection, and succeeds. It's not obvious how you can persuade to it do otherwise. Because validation succeeds, it attempts to then write the settings file, which then fails on a file permissions exception, but the point where we wanted to generate the exception for the test was during validation.

I'll look around a bit more if there's smaller bit of connection logic we can test, or if I've missed a way of resetting the database connections (maybe read them in from settings and unset them one by one via Database::removeConnection?)