[Corona] Add extended routing functionality to the FrontController class

Review Request #384 — Created Aug. 5, 2015 and submitted

pprkut
Lunr
master
a6f79a4...
lunr
Add extended routing functionality to the FrontController class.
Unit tests
  • 0
  • 0
  • 6
  • 2
  • 8
Description From Last Updated
tardypad
  1. I find the following confusing and potentially cause of error:
    - we will register paths for libraries in the order we want to lookup
    - we will get a FQDN of the controller that we need from the route function
    - we will instantiate that controller afterwards
    BUT
    the controller instantiated will be defined based on the include_path and not the paths registrations.
    
    Would that make sense to have the set_include_path inside the paths registration?
    1. This is only a problem if you have conflicting or no namespaces. Otherwise you can have a controller with the same name, but the FQCN would be different.
      I don't think setting up the include path inside the FrontController makes any sense. For one, the include path needs to be set up already to be able to instantiate the FrontController class.
  2. src/Lunr/Corona/FrontController.php (Diff revision 1)
     
     
    typo "Identifiers"
  3. src/Lunr/Corona/FrontController.php (Diff revision 1)
     
     
    shouldn't this better be protected?
    1. I didn't see a reason to hide it. If someone wants slightly different routing behavior than available in route(), the lookup() function would still be accessible from index.php.
      I don't know if that's valid a reason enough to keep it public, but I couldn't come up with a good enough reason to make it protected either.
  4. src/Lunr/Corona/FrontController.php (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    define "$controller = '';" before the foreach for in case there is no path at all.
    
    Or "return '';" in the end of the function and "return $controller;" instead of the "break;"
  5. src/Lunr/Corona/FrontController.php (Diff revision 1)
     
     
    $this->paths[$id] is potentially not set
  6. maybe add a third paths in here, just to test that the lookup stops as soon as it finds a controller
  7. I think we should have an extra failure test in case no path is defined at all.
  8. I think it is confusing to use foo/bar everywhere
    the first 'foo/bar' is meant for 'controller/method' while the second one is a folder path
    
    That makes the tests difficult to read
  9. 
      
pprkut
tardypad
  1. 
      
  2. src/Lunr/Corona/FrontController.php (Diff revision 2)
     
     

    2016 :D
    and in the unit tests if we care

  3. 
      
tardypad
  1. Ship It!
  2. 
      
pprkut
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master

Loading...