theseer / Autoload

A lightweight php namespace aware autoload generator and phar archive builder
Other
388 stars 47 forks source link

Implemented check to prevent exceptions when Redeclaration of Classes is detected in the same file #12

Closed gooh closed 13 years ago

gooh commented 13 years ago

When a class is conditionally declared in a file, like in

if (class_exists("Zend_Exception")) {
    abstract class Zend_InfoCard_Exception_Abstract extends Zend_Exception {}
} else {
    abstract class Zend_InfoCard_Exception_Abstract extends Exception {}
}

phpab will throw an exception with a message like

Redeclaration of class 'Zend_InfoCard_Exception_Abstract' detected
Original: /path/to/Zend/InfoCard/Exception/Abstract.php
Secondary: /path/to/Zend/InfoCard/Exception/Abstract.php

While the conditional declaration might be questionable practise, there is no reason for phpab to end classmap generation in that case. There is no conflict because the source files are the same.

The attached patch will check if a class that was already found is in the same file and if so, simply skip to the next file. Only if the source files are not identical, the exception is thrown.


The second patch fixes a test that will cause an error on Windows due the non-existing /tmp path there.

theseer commented 13 years ago

Thanks a lot for the Code. Two questions:

097793c (Redeclaration): While that code will obviously work, I'm not too fond of silently skipping. There should be a notification of some sort - even if the processing will continue. Optionally it would be actually a switch to toggle that behavior, having the default bailing out.

3a8ea60 (OS independent tests): Despite the nicer path for storing the temp file, I don't see any actual modifications of code that would have any effect - let alone fix potential OS related issues?

gooh commented 13 years ago

Re 097793c (Redeclaration):

I thought about triggering a warning or a notice, but wasn't sure because phpab is not phpcs. I agree that the example code is poor practise. But this particular redeclaration is on purpose and even it if wasn't, it will work without any issues in the classmap. If I was the author of the code I'd already know there is a Redeclaration, so no need to tell me.

Likewise, telling users about the Redeclaration when they just want to create a working classmap for their project isn't helping either. They are unlikely to modify the offending code. We are effectively just telling them: "hey, the 3rd party lib you want to use is stupid there". So why tell them?

Then again, making this an optional switch will probably not hurt. Though I think adding it is more work than what we are gaining from it. But just let me know. If you want it desperately, I'll add it.

Re 3a8ea60 (OS independent tests):

PHPUnit will give an error when running AutoloadBuilderTest::testSaveFile() on Windows because /tmp does not exist on Windows. By changing the path to sys_get_temp_dir() we make sure that the test will pass on any OS. The call to uniqid() just makes sure there is no clashes with any existing test.php files in temp (might be unlikely to happen but since I was already at it …). An alternative could be to supply the save path in the test with php://memory or php://temp.

Any of the changes outside the testSaveFile method do nothing. That was just my IDE cleaning whitespace. Sorry for that.

theseer commented 13 years ago

Re 097793c (Redeclaration): Okay, point taken. Let's go with the easy solution for now. Re 3a8ea60 (OS independent tests): Makes sense, didn't see that.