Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor callable resolver, router, route, and route group #2098

Merged
merged 13 commits into from Dec 11, 2016

Conversation

Projects
None yet
3 participants
@codeguy
Copy link
Member

commented Dec 10, 2016

  1. Remove CallableResolverTrait
  2. Inject resolver into DeferredCallable instances instead of container
  3. Set default callable resolver when appropriate if custom resolver not provided
  4. Refactor Route, Router, RouteGroup to use new callable resolver
  5. Update older service provider to inject resolver into router during creation
  6. Add router setter/getter methods to App
  7. Add callable resolver setter/getter methods to App

Please review carefully before merging. This is a big change. I didn't touch some places in App that still rely on the presence of a container, such as where we need app settings, or response handlers (Found, NotFound, etc.) and so on. Those are on my list. I've also sprinkled TODO notes throughout.

@codeguy codeguy added the Slim 4 label Dec 10, 2016

@codeguy codeguy added this to the 4.0 milestone Dec 10, 2016

@codeguy codeguy requested a review from akrabat Dec 10, 2016

@coveralls

This comment has been minimized.

Copy link

commented Dec 10, 2016

Coverage Status

Coverage decreased (-1.08%) to 96.803% when pulling 2f792ba on codeguy:feature-refactor-callable-resolver into 0f0c8c5 on slimphp:4.x.

@codeguy codeguy referenced this pull request Dec 10, 2016

Closed

Refactor CallableResolver #2096

@coveralls

This comment has been minimized.

Copy link

commented Dec 10, 2016

Coverage Status

Coverage decreased (-1.08%) to 96.803% when pulling 767a9c9 on codeguy:feature-refactor-callable-resolver into 0f0c8c5 on slimphp:4.x.

$resolver = null;

// Fetch resolver from container if container is set
if ($this->container instanceof ContainerInterface) {

This comment has been minimized.

Copy link
@akrabat

akrabat Dec 10, 2016

Member

Should we be using the container at all here? i.e. if you want to change the callable resolver, then you call setCallableResolver().

This comment has been minimized.

Copy link
@codeguy

codeguy Dec 10, 2016

Author Member

I think my intention was to retain temporary BC with our current container (to prefer what it provides). But I agree with you. I just pushed a new commit to remove it from this method.

@coveralls

This comment has been minimized.

Copy link

commented Dec 10, 2016

Coverage Status

Coverage decreased (-1.0%) to 96.911% when pulling f9f4953 on codeguy:feature-refactor-callable-resolver into 0f0c8c5 on slimphp:4.x.

$router = null;

// Fetch router from container if container is set
if ($this->container instanceof ContainerInterface) {

This comment has been minimized.

Copy link
@akrabat

akrabat Dec 10, 2016

Member

Same here, I think?

This comment has been minimized.

Copy link
@codeguy

codeguy Dec 10, 2016

Author Member

Just pushed a new commit. Luckily, this brought several affected unit tests to my attention.

@coveralls

This comment has been minimized.

Copy link

commented Dec 10, 2016

Coverage Status

Coverage decreased (-0.2%) to 97.639% when pulling dee7269 on codeguy:feature-refactor-callable-resolver into 0f0c8c5 on slimphp:4.x.

@akrabat

This comment has been minimized.

Copy link
Member

commented Dec 10, 2016

https://github.com/akrabat/slim-bookshelf breaks when I use this branch with it. I get:

An exception has been thrown during the rendering of a template ("Named route does not exist for name: list-authors") in "layout.twig" at line 35.

@codeguy

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2016

@akrabat See:

https://github.com/akrabat/slim-bookshelf/blob/master/app/src/dependencies.php#L17

You'll need the app's router with $app->getRouter(), else inject your container's router with $app->setRouter($c['router']).

@codeguy

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2016

@akrabat Should we have a method, such as, $app->loadFromContainer($container) that populates major objects from a third-party container to help with upgrading from v3?

@codeguy

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2016

Example code:

public function loadFromContainer(ContainerInterface $c)
{
    if ($c->has('router')) {
        $this->setRouter($c->get('router'));
    }
    // repeat for other objects
}
@codeguy

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2016

We could even do importFromContainer($container) and exportToContainer($container) to expedite stuff both ways.

@akrabat

This comment has been minimized.

Copy link
Member

commented Dec 11, 2016

Aha! Yeah - that's an obvious issue...

@akrabat

This comment has been minimized.

Copy link
Member

commented Dec 11, 2016

I'm not sure we need helper methods yet. Maybe wait and see what happens in beta when people try upgrading?

Certainly we should document this in the upgrading documentation as a minimum. Maybe create an UPGRADING.md file in root so that we can note while we we work?

For now, I think that the skeleton's index.php needs:

$app->getContainer()['router'] = $app->getRouter();
@codeguy

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2016

Just pushed an UPGRADING.md file to document changes.

@coveralls

This comment has been minimized.

Copy link

commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.2%) to 97.639% when pulling c9c37a5 on codeguy:feature-refactor-callable-resolver into 0f0c8c5 on slimphp:4.x.

@@ -0,0 +1,5 @@
# How to upgrade

* [2098] - You must inject the container router into app with `$app->setRouter()`

This comment has been minimized.

Copy link
@akrabat

akrabat Dec 11, 2016

Member

Technically, you need to add the App's router to the container for a straight upgrade. If you've created your own router factory in the container though, then you need to set it into the $app.

This comment has been minimized.

Copy link
@codeguy

codeguy Dec 11, 2016

Author Member

Updated language

@coveralls

This comment has been minimized.

Copy link

commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.2%) to 97.639% when pulling cd1a3e3 on codeguy:feature-refactor-callable-resolver into 0f0c8c5 on slimphp:4.x.

@akrabat akrabat merged commit cd1a3e3 into slimphp:4.x Dec 11, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 97.639%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

akrabat added a commit that referenced this pull request Dec 11, 2016

@l0gicgate l0gicgate referenced this pull request Apr 25, 2019

Merged

Slim 4 Alpha Release #2665

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.