[Core] Adding PSR-4 support

Review Request #397 — Created Nov. 16, 2015 and submitted

smillernl
Lunr
personal/sean/psr4_attempt2
04b3e9d...
lunr

Core: Adding PSR-4 support



  • 0
  • 0
  • 7
  • 0
  • 7
Description From Last Updated
pprkut
  1. 
      
  2. src/Lunr/Core/Autoloader.php (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    I don't think this works correctly. It only checks for a full match of the namespace, but not for partial matches. Here is an example data provider for a unit test (which is also missing, btw):

        public function psr4Provider()
        {
            $classes   = [];
            $classes[] = [ 'L1\L2', 'src/Level/', 'L1\L2\L3\L4\File', 'src/Level/L3/L4/File.php' ];
            $classes[] = [ 'L1\L2', 'src/Level/', 'L1\L2\L4\File', 'src/Level/L4/File.php' ];
            $classes[] = [ 'L1\L2', 'src/Level/', 'L1\L2\File', 'src/Level/File.php' ];
            $classes[] = [ 'L1\L2', 'src/Level/', 'L1\File', 'L1/File.php' ];
    
            return $classes;
        }
    

    based on this I came up with the following implementation:

        public function get_psr4_class_filepath($class)
        {
            $namespace = substr($class, 0, strrpos($class, '\\'));
            $classname = str_replace('\\', DIRECTORY_SEPARATOR, $class);
            $class     = substr($class, strrpos($class, '\\') + 1);
    
            do
            {
                if (isset($this->prefixes[$namespace]) === TRUE)
                {
                    $base_path = $this->prefixes[$namespace];
                    $class     = str_replace('\\', DIRECTORY_SEPARATOR, $class);
    
                    return $base_path . $class . '.php';
                }
                else
                {
                    $class     = substr($namespace, strrpos($namespace, '\\') + 1) . '\\' . $class;
                    $namespace = substr($namespace, 0, strrpos($namespace, '\\'));
                }
            }
            while (strrpos($namespace, '\\') > 1);
    
            return $classname . '.php';
        }
    
  3. src/Lunr/Core/Autoloader.php (Diff revision 1)
     
     

    we're not setting a namespace, we're setting the path for a namespace prefix, so IMHO "set_prefix" would be a better name.

  4. src/Lunr/Core/Autoloader.php (Diff revision 1)
     
     

    adding an include path should never happen automagically. This can cause all kinds of really hard to debug problems.

  5. 
      
smillernl
tardypad
  1. I think the unique AutoloaderTest.php file deserves to be split now :/

    I think we miss some testing with multiple prefixes set (to test the order mainly)

    Let say for example we have as prefixes
    - Foo\Bar => /libs/yoyo
    - Foo\Bar\Baz => /libs/hehe
    and as files
    - /libs/yoyo/Baz/Qux.php
    - /libs/hehe/Qux.php
    then I think
    \Foo\Bar\Baz\Qux
    should resolve to
    the hehe libs file

  2. src/Lunr/Core/Autoloader.php (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    I think psr4 should be looked at first
    since this is the "official" one now

    in case we have 2 files:
    - libs/Foo/Bar_Baz.php
    - libs/Foo/Bar/Baz.php

    \Foo\Bar_Baz should resolve to the first file
    while I think it will resolve to second one with the current code

    This order thing should be unit tested as well then

    1. If it looks at PSR4 first it'd never get to use PSR1 as PSR4 is PSR1 compliant afaik.

    2. PSR-4 is not PSR-0 compliant. That's the whole point why some projects refuse to switch. It's PSR-0 without legacy class name support (and added prefix support).

      Anyway, the current assumption is that PSR-0 takes care of exact matches (i.e. classes that can be loaded without needing a prefix defined). If we decide to do PSR-4 first,
      we quite likely need a completely different implementation that, still, checks for exact matches first, but ignores legacy class name matching either entirely or until after PSR-4 handling.

    3. reading the PSR-4 specs again, I'm not even sure what it's supposed to do with exact matches that don't need prefixes...

    4. Yay or nay on changing the order? Heinz put PSR-0 first and damien says PSR-4 first. Can I demand a trial by combat to get this settled?

      I personally think that the PSR-0 part of PSR-4 would be used in most situations anyways so no need to get the complex loader first.

    5. Please make the unit tests and see the implications of one over the other

  3. src/Lunr/Core/Tests/AutoloaderTest.php (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     

    I think we should have more than these
    - use of underscore in namespace
    - use of underscore in class name
    - multiple levels of partial class
    - multiple levels of partial path
    -...

    1. Underscores shouldn't be picked up in the PSR-4 loader right? so that should be a PSR4ErrorProvider

    2. This is not an "error", it should be picked as such in PSR4 always
      And not replaced by a directory separator as in PSR0, if it was in the class name
      AFAIR

  4. src/Lunr/Core/Tests/AutoloaderTest.php (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    you never use $fullClass or $fullPath in here
    You should use a different provider for this function if needed or none at all

    1. You can just leave of the parameters from the function definition. PHP doesn't care if the data provider sends more parameters than we use.

  5. src/Lunr/Core/Tests/AutoloaderTest.php (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    I have a bit of trouble seeing what this test more than the previous function
    Either remove or make the description more precise

  6. 
      
smillernl
smillernl
smillernl
pprkut
  1. Ship It!
  2. 
      
smillernl
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master

Loading...