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

Create a separate extension for helper functions #22

Merged
merged 3 commits into from
Jul 6, 2015

Conversation

akrabat
Copy link
Member

@akrabat akrabat commented Jul 5, 2015

Also add base_url Twig function.

@akrabat akrabat mentioned this pull request Jul 5, 2015
@silentworks
Copy link
Member

The method name seem to communicate one thing while doing something else IMO. I would actually prefer to register the extension manually and doing it externally.

@akrabat
Copy link
Member Author

akrabat commented Jul 5, 2015

You mean registerExtension ?

I kept that so that the set up of the extension remains:

$container->register(new \Slim\Views\Twig('path/to/templates', [
    'cache' => 'path/to/cache'
]));

which is nice and simple to use. I'm not wedded to it though and am happy enough to remove the method and change the README.

@silentworks
Copy link
Member

if thats the case its probably better to make that method private, Having it as public confuses me.

Although saying that, I still think most users are happy to register it themselves as this is how Slim-Views currently works plus it is easier to override path_for and base_url with my own extension.

Also add base_url Twig function.
@akrabat
Copy link
Member Author

akrabat commented Jul 5, 2015

Rebased, removed registerExtension and updated README.

@silentworks
Copy link
Member

The README doesn't look correct to me.

@akrabat
Copy link
Member Author

akrabat commented Jul 5, 2015

Try now!

silentworks added a commit that referenced this pull request Jul 6, 2015
Create a separate extension for helper functions
@silentworks silentworks merged commit 3a8216f into slimphp:master Jul 6, 2015
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.

None yet

2 participants