Classmap implementation #4
Conversation
src/Replace/ClassmapReplacer.php
Outdated
| return preg_replace_callback( | ||
| '/(?:\W[abstract]*class |interface )([a-zA-Z\_]+)[ ]*{/U', | ||
| function ($matches) { | ||
| $replace = 'CJDT_' . $matches[1]; |
There was a problem hiding this comment.
The CJDT_ prefix is hardcoded here, needs to be replaced by a config variable.
src/Composer/Autoload/Classmap.php
Outdated
| class Classmap implements Autoloader | ||
| { | ||
| /** @var array */ | ||
| public $paths = []; |
There was a problem hiding this comment.
Classmap autoloader should support having files in the array, not just directories. Potentially look into splitting the $paths variable up in $directories and $files.
src/Mover.php
Outdated
| $targetFile = str_replace($this->workingDir, $this->config->dep_directory, $file->getRealPath()); | ||
| } else { | ||
| $namespacePath = str_replace('\\', '/', $autoloader->namespace); | ||
| if (is_a($autoloader, NamespaceAutoloader::class)) { |
There was a problem hiding this comment.
You should prefer instanceof operator instead of is_a() function. The operator doesn't incur a function overhead, and is_a() was only de-deprecated because of popular request. It should have been gone already.
There was a problem hiding this comment.
Thanks, fixed: 05fea7d
src/Mover.php
Outdated
|
|
||
| foreach( $this->replacedClasses as $original => $replacement ) { | ||
| $contents = preg_replace_callback( | ||
| '/\W('.$original.')(?:\(|\:\:)/U', |
There was a problem hiding this comment.
This will miss a couple of scenarios.
The ones I can think of right now:
- type-hints:
public function __construct( MyClass $obj ); newwithout arguments:$obj = new MyClass;- class in string:
$class = 'MyClass'; $obj = new $class();
It will also probably break when using relative namespaces, like the following:
namespace Vendor;
throw new Exception\MyException(); // FQCN: \Vendor\Exception\MyException
I'm not really sure how you can add cases like these without getting false positives as well, though. I don't think this can properly be handled by regexes, you should look into using nikic/PHP-Parser perhaps.
There was a problem hiding this comment.
Thanks @schlessera, I would love to make this as bullet proof as I can, but don't want this to distract too much from actually implementing the classmap handler. Improving the replacing logic can obviously be an ongoing process. Can you please post this as an issue, so I can handle it accordingly ( - it would be amazing if the GitHub UI allowed me to promote this comment to an issue, directly from here)?
Revert "All class directories will now be prefixed with vendor/package" This reverts commit c91d232.
This obviously needs to be cleaned up a ton before it's being used out in the wild, but it's good enough for demonstration purposes...
Props to @dannyvankooten for helping me out on this
My first stab at getting #2 ready. A little rough around the edges, but ready for a second pair of eyes if anyone has a couple minutes.