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

forgot to add the extratokens variable to the serve function #61

Merged
merged 1 commit into from
Sep 25, 2013

Conversation

small1
Copy link
Contributor

@small1 small1 commented Sep 24, 2013

Added $extratokens to the server() fuction.

Sample usage:
Toro::serve(array(
"/" => "MainPageHandler",
"/users" => "UsersHandlerShow",
"/users/:userid/edit" => "UsersHandlerEdit",
), array(':userid' => CurrentUserID())); // CurrentUserID() in this case returns the userid of the currently logged in user.

martinbean added a commit that referenced this pull request Sep 25, 2013
Added $extratokens variable to serve method
@martinbean martinbean merged commit b1e76b0 into anandkunal:master Sep 25, 2013
@martinbean
Copy link
Collaborator

Thanks. Good idea, I think I’m going to re-factor this so tokens can be added before the serve call, as I don’t think any extra tokens should be passed along with that method.

@jmashburn
Copy link

Based on the examples given on the wiki, the code is now broken. If I don't pass extratokens into the "serve" extratokens defaults to null and throws an error when attempting the foreach loop.

It seems to me that what is trying to be accomplished here can already be done with the current (pre: merge) of Toro. I would suggest removing this new code and putting it into your own "HandlerClass" using the ToroHooks as illustrated here: https://gist.github.com/jmashburn/6704746

@small1
Copy link
Contributor Author

small1 commented Sep 25, 2013

This code should have been two separate commits.
One hasHandlerFor and one for the extratokens.
If the extratokens code is removed everything would be back to normal without throwing errors. And the extratoken thing should has martinbean says that it should be added before the serve call.

@jmashburn
Copy link

Ok, I'll watch for the change.
As for the hasHandlerFor function, whats it's purpose? Looking at your example above it seems that you want to create static routes /users/<< userId >>/edit. Doesn't /users/:number/edit do the same thing?

@martinbean
Copy link
Collaborator

My bad. I’ve not been as sharp as I could have been the last couple of days as I’ve picked up a bug.

I’ve created a new branch called develop with the broken code, and rolled back master to pre-Pull Request. I’ll work on adding the “extra tokens” functionality to Toro’s core in a more elegant manner.

@jmashburn From what I understand, @small1 wanted to check if there was a handler that matched a passed URL.

@jmashburn
Copy link

@martinbean Did you see the Gist I added earlier in the thread? Seems like a perfect example of the ToroHooks passing the routes into the callbacks which can be assigned in the HandlerClass and checked there or am I not seeing the bigger picture?

@martinbean
Copy link
Collaborator

@jmashburn I hadn’t. It looks clean and tidy, but think @small1 was wanting to add his own tokens that could be used in route patterns, like :number etc. Although saying this, I would do pretty much what you do in your Gist, doing the condition check within my handler. M

@jmashburn
Copy link

I created a branch at https://github.com/jmashburn/ToroPHP/tree/Tokens with my attempt at tokens/routes.

Here is an example how it works. < https://gist.github.com/jmashburn/6708647 > You can add additional routes/tokens in the before_request callback, or statically, before the serve call or a combination of any/all.

There is minimal code changes in the core. Function's like has hasHandlerFor can be put in callbacks and parse the routes from the params that are passed in.

What do you think?

@martinbean
Copy link
Collaborator

I was thinking of moving the $tokens variable into a static class property that could then be added to with a static method, something like Toro::addTokens(array(':userid' => currentUserId()));.

@jmashburn
Copy link

Check my branch. I''ve already added that. https://github.com/jmashburn/ToroPHP/tree/Tokens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants