Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Provide template factories and facilities #128

Merged

Conversation

weierophinney
Copy link
Member

This patch aims to address #122 and #123, by providing container factories for each template engine, as well as useful facilities for integrating them with Expressive.

This patch imports the factories introduced at:

- https://github.com/xtreamwayz/expressive-composer-installer/tree/master/src/App/Template

for each of Twig, Plates, and ZendView, with modifications (particularly the
lack of default template paths). Additionally, unit tests have been added to
verify behavior of each container.

The Twig support now includes the TwigExtension, which overrides the path and
asset twig helpers in order to delegate to zend-expressive functionality.
- The Whoops runtime should now be injected with a pretty page handler,
  per zendframework#125.
@weierophinney weierophinney added this to the 0.3.0 milestone Sep 10, 2015
@weierophinney weierophinney self-assigned this Sep 10, 2015
@weierophinney weierophinney changed the title Provide template factories and facilities [WIP] Provide template factories and facilities Sep 10, 2015
@weierophinney
Copy link
Member Author

Ping @xtreamwayz - still in progress, but I think this will make the work in the installer easier. :)

@codeliner
Copy link
Contributor

thanks to @xtreamwayz and @weierophinney for this. We are looking forward to the zend-view integration especially set up of the HelperPluginManager. I reviewed the current state of the PR but I was not able to figure out how the UrlHelper can be registered in the view. But state is wip.

We currently use three factories to set up zend-view:
https://github.com/prooph/proophessor-do/tree/master/src/Container/App/View

Whereby the PhpRendererFactory logic can be merged into the ZendViewFactory logic like shown in this PR. However, the ViewHelperPluginManagerFactory registers the app container (currently only possible if it is an instance of Zend\ServiceManager\ServiceManager, maybe we should change that in zend-view?) as a peering manager and uses it first to locate plugins. Problem is of course that the service name of a view plugin must match with the short name of the plugin so naming collisions can cause trouble.
https://github.com/prooph/proophessor-do/blob/master/config/services.php#L51

@geerteltink
Copy link
Member

@weierophinney I've made an installer test branch here: composer-installer/feature/template-factories

I was already hoping you would add the template factories. Thanx a lot.

Everything is working out of the box now. One issue tough:

The default extension for twig should be .html.twig. This is not a big deal but it's better than .html. Mainly because IDE's pick up the .html.twig extension and the other reason is listed here: The template name also has two extensions that specify the format and engine for that template.

Also I used zend-config for the configuration container. A class is needed to inject the config into Aura.Di. But zend-config fails on the is_array check. Fortunately the standard ArrayObject class does work, which will be used now. It also removes the zend-config dependency.

@codeliner The url helper class and tests are there, but it's not injected yet. I guess you have to wait until the box in front of this is ticked: Push into helper manager via ZendViewFactory

@codeliner
Copy link
Contributor

@xtreamwayz damn, of course I didn't read carefully enough. However, I'm excited about the solution. As noted above we had a hard time figuring out a way to get a custom view helper into zend-view when only working with a application wide container-interop container. Currently, we have a hard dependency to zend-servicemanager. Using pimple-interop instead won't work at the moment.

But yeah, let's wait until the box is ticked.

- Consumes PSR-7 request URI.
The zend-view container factory now also injects the HelperPluginManager
instance, adding overrides for the url and serverurl helpers.
@weierophinney weierophinney changed the title [WIP] Provide template factories and facilities Provide template factories and facilities Sep 12, 2015
@weierophinney
Copy link
Member Author

@xtreamwayz

The default extension for twig should be .html.twig.

Fixed!

Also I used zend-config for the configuration container. A class is needed to inject the config into Aura.Di. But zend-config fails on the is_array check. Fortunately the standard ArrayObject class does work, which will be used now. It also removes the zend-config dependency.

Is there an action item here? Or were you indicating that the approach I used solves an issue you had in your original implementation?

@codeliner

The zend-view factory now will inject the custom view helpers into the helper plugin manager.

@weierophinney weierophinney merged commit df44ea1 into zendframework:develop Sep 12, 2015
weierophinney added a commit that referenced this pull request Sep 12, 2015
@weierophinney weierophinney deleted the feature/template-factories branch September 12, 2015 17:49
weierophinney added a commit that referenced this pull request Sep 12, 2015
@geerteltink
Copy link
Member

Is there an action item here? Or were you indicating that the approach I used solves an issue you had in your original implementation?

@weierophinney I actually wanted to tell you that the is_array check gave errors. When writing it down I thought of a different way to get it working. So it was more an action for me.

@codeliner
Copy link
Contributor

@weierophinney Thx! Works like a charm.

I added an additional plugins config key to the templates config
to be able to inject custom view helpers into the HelperPluginManager.
Could be a nice addition to the ZendViewFactory?

@weierophinney
Copy link
Member Author

Sure; send a pull request. :-)
On Sep 12, 2015 3:29 PM, "Alexander Miertsch" [email protected]
wrote:

@weierophinney https://github.com/weierophinney Thx! Works like a charm.

I added an additional plugins config key to the templates config
https://github.com/prooph/proophessor-do/blob/master/config/templates.php#L26
to be able to inject custom view helpers into the HelperPluginManager
https://github.com/prooph/proophessor-do/blob/master/src/Container/App/View/ViewHelperPluginManagerFactory.php#L35
.
Could be a nice addition to the ZendViewFactory?


Reply to this email directly or view it on GitHub
#128 (comment)
.

@bakura10
Copy link
Contributor

Hi,

I may be completely wrong, but if I look at that: https://github.com/zendframework/zend-expressive/blob/master/src/Container/Template/TwigFactory.php#L49

it looks to me that you are basically expecting this to be in the main config array returned by "config". This seems a bit fragile to me. I mean, all the key names "debug", "templates" are super common and you could have a lot of weird bugs when everything will be merged at some point into the "config" service.

I'd advocate that we should adopt a three levels structure for the container: "component" > "sub-component" > "option", so in this case the config expected would be:

return [
    'expressive' => [
       'twig' => [
          'debug' => // ...
      ]
   ]
]

@geerteltink
Copy link
Member

The debug setting is inherited from my installer. The reason behind it is that your app is in dedug mode or not. It makes sense to move it into the temple array though if there is a need to debug only your templates.

Normally you would be right with a structure like that. But I don't think expressive is the component in your example. The template engine is the component. Expressive just provides the factories for your convenience. And template is just a common name for all the different engines supported by expressive.

And so far I haven't found any weird bugs with this naming. I guess it might be an issue when you use 2 different template engines at the same time for whatever reason.

@bakura10
Copy link
Contributor

Hi,

Thank you for the clarification. It seems that indeed the "debug" is a global Zend Expressive config. I'd say that we need to have a clear way to structure config, and I like the component > sub-component > config way (this is what @mnapoli suggested too iirc).

This would open the path to use something like Puli for merging config (which should be top priority at the moment in my opinion :D).

By following this logic, it shoudl look like this:

return [
   'expressive' => [
       'debug' => true,
   ],

   'aura' => [
       'router' => [
            // Router config
       ]
   ],

   'fast_route' => [
      'router' => [
         // Router config
      ]
   ],

   'zf2' => [
      'templates' => [
           // ZF2 templates
      ],

      'router' => [
          // ZF2 router
      ]
   ],

   'twig' => [
      'templates' => [
          // Twig templates
      ]
   ]
];

And so on... Not only this make thing much coherent, and encourage using the same style for third-party library (as well as providing a common structure that would work even when merging config from unknown third parties), but it would also allow to have templates both from ZF2 and Twig transparently, for instance.

I find it a bit strange that templates are routes seems to be configured quite differently in Zend Expressive.

What do you think @weierophinney ?

@codeliner
Copy link
Contributor

👍 for @bakura10 suggestion. We successfully used interop-config which suggest the same way to structure component config. In our example application we used it to write a factory for Doctrine DBAL.

But I think routes and templates need to be placed either under a application root config key or the expressive config key because you don't configure the twig or aura component directly but the wrapper provided by zend-expressive. You can exchange the router for example and the routes will continue to work (as long as you don't use vendor specific options like passing values to Aura.Router routes)

@bakura10
Copy link
Contributor

Definitely. What confuses me a bit is the fact that routes are actually configured as part of the Expressive application factory, while the templates have their own dedicated factory. So maybe we could use a unifided system as you suggested:

return [
   'expressive' => [ // Or application
     'routes' => [

     ],

     'templates' => [

     ]
  ]
];

Then, instead of having the routes configuration pulled here: https://github.com/zendframework/zend-expressive/blob/master/src/Container/ApplicationFactory.php#L152

We would instead have a dedicated factory for AuraRouter, Zf2Router and FastRouteRouter that would fetch them. This would make this more coherent with how templates work.

@codeliner
Copy link
Contributor

@bakura10 good catch. 👍 for dedicated factories.

@weierophinney
Copy link
Member Author

There's a key difference between templates and routes however: Application
requires a router, but not templates. (APIs, for instance, would not need
templates.)

Another difference is that for template paths, we can operate on the
adapter, not the underlying engine, and this is a better approach as
adapters abstract the path/namespace detail. As such, separating the
configuration by adapter type is ignoring the abstraction.

Routers are more likely to benefit from adapter specific configuration due
to the differences between them. Contrary to comments in this thread, they
are not interchangeable; the route syntax varies widely between them, and
none of them share any commonalities. The only part that is similar between
them is that we have a common way of defining application routes due to
having abstracted the requirements (path, middleware, methods, name, and
options). This is the reason they can share the same factory. However,
you'll note that the factory allows pulling the underlying engine from the
container, allowing it to have separate configuration prior to injection in
the abstraction.

I definitely see the point of standardizing the configuration; I've not
seen a proposal here that makes sense with the current architecture yet,
and need to do some brainstorming on workable structures before committing
to a specific direction.
On Sep 13, 2015 6:36 AM, "Michaël Gallego" [email protected] wrote:

Definitely. What confuses me a bit is the fact that routes are actually
configured as part of the Expressive application factory, while the
templates have their own dedicated factory. So maybe we could use a
unifided system as you suggested:

return [ 'expressive' => [ // Or application 'routes' => [ ], 'templates' => [ ] ]];

Then, instead of having the routes configuration pulled here:
https://github.com/zendframework/zend-expressive/blob/master/src/Container/ApplicationFactory.php#L152

We would instead have a dedicated factory for AuraRouter, Zf2Router and
FastRouteRouter that would fetch them. This would make this more coherent
with how templates work.


Reply to this email directly or view it on GitHub
#128 (comment)
.

@codeliner
Copy link
Contributor

@weierophinney

Contrary to comments in this thread, they
are not interchangeable; the route syntax varies widely between them, and
none of them share any commonalities

Good to know that. Based on the docs my assumption was that they are interchangeable.

But yeah, only static routes will continue to work, dynamic ones require small to medium adjustments.

@weierophinney
Copy link
Member Author

I'm doing a little analysis here, and I'm a bit stuck, as we have a few things going on:

  • Abstractions for application concerns (containers, routing, templating).
  • Configuration targeting the abstractions.
  • Implementation-specific configuration.

The ApplicationFactory depends on having a RouterInterface service present, and chooses a default (Aura.Router) if none is provided. It then injects routes, which are an abstraction. Thus, if you have implementation-specific concerns, you are expected to write your own factory, using whatever configuration you need, and inject that into the container. This is clean, and addresses the three factors well. The only questionable aspect at this point is where the "routes" key should live: should it be top-level (as it addresses the "router" component), or do we push it under the "zend-expressive" key?

Next, we have the Zend\Expressive\FinalHandler meta-service. This one deals once again with all three aspects: FinalHandler is an abstraction, we have abstract configuration (debug mode, templates, etc.), and we have implementation-specific configuration (whoops). The FinalHandler configuration story most closely follows what @bakura10 has suggested, as it has top-level "zend-expressive" and "whoops" keys, and a second-level "error_handler" key for the expressive configuration.

Finally, we have templates. These differ from the other concerns in that:

  • Templates are completely optional.
  • 2 of the 3 offerings have Expressive-specific features to aid in integration.
  • The factories do not allow pulling existing services for the underlying engines to inject in the abstractions.

Like routes, however, there is an abstraction-specific aspect: paths. My thought on this is to make them follow the routing paradigm for configuration: each will attempt to pull the templating engine from the container if possible, allowing for component-specific configuration if desired and using a default instance if no service is found. From there, paths will be injected into the template abstraction.

One other aspect that differs with templates is that two of the three implementations have additional constructor arguments that are specific to their implementations of the abstraction (e.g., layout for ZendView). As such, they will also need expressive-specific configuration, in addition to potential component/engine-specific configuration. This would need to live under a "templates" or "template" sub-key of the expressive configuration.

This leaves us with exactly one decision to make: where should the "routes" and "templates" configuration live? Do we provide a top-level "zend-expressive" key and push them under that? or keep them as top-level (as they are now)?

Regardless, I'll start working on the changes to the template factories a noted above (allowing pulling the engine as a service).

@geerteltink
Copy link
Member

First of all, I'm writing this as a developer of the expressive installer. If you haven't seen it yet, please have a look.

There are a few namespaces we are talking about here. They are:

  • dependencies / container
  • middleware_pipeline
  • routes
  • templates
  • whoops
  • debug
  • zend-expressive

Explanation:

  • middleware_pipeline: It's just that. Can move it into a expressive namespace since it is just a part of expressive.
  • routes: Same. They are routes. Keep it simple. However routes are configured differently for each router. You could use aura_router or zend-mvc-router-stack. But at the end they are still routes.
  • templates. Same as above.
  • whoops. Same as above, however it's the only optional error_handler.
  • debug is something I introduced with the installer. The idea was to set debug once and debug the whole app. I can imagine someone wants to debug only twig. I don't mind that it is moved into a template namespace.
  • zend-expressive is what it is. It might be renamed to expressive since it's pretty unique.

As you can see, the only thing that I didn't mention is the container. This is basically outside the scope of zend-expressive. It expects a container-interop, but it depends on the user on how to load and set it up. Besides that, for the sake of simplicity of the installer, it has to be the same name for all different containers. Just have a look here so you can see how it relies on 1 namespace for the container between all related dependencies.

Having said all this, if you need a proper namespace, I suggest something like this:

return [
    'dependencies or container' => [
    ],
    'zend-expressive or expressive' => [
        'middleware or middleware_pipeline' => [],
        'routes' => [],
        'templates' => [
            'debug' => BOOLEAN // Just for twig
        ],
        'whoops or error_handler' => [],
    ],
];

AS I said before, the container is out of the scope of expressive. So basically it's up to the installer or the user to name it. For the installer it has to be a unified name for simplicity.

I don't know what the standard whoops config is, but if it is different, just place it inside the expressive namespace. Same goes for other config. If it is a config forced by expressive or its factories, add it to the expressive namespace.

I know this sounds strange to zend-framework users since zend-framework has an excellent namespacing. However, this is not zend framework. As I understand, expressive is a middleware micro-framework that relies on diactoros and stratigility. Besides that, it makes a difference from other middleware frameworks by supporting multiple php packages.

I'm just saying, keep it as simple as possible to attract a bigger audience.

@weierophinney
Copy link
Member Author

Finally took a look at interop-config, and it looks quite interesting, in part because it may simplify a ton of the logic in the factories themselves. Will investigate as I start refactoring; thanks, @codeliner !

@xtreamwayz — I agree with keeping it as simple as possible. However, I also think consistency of configuration is important (it's inconsistent currently; see my notes on error handling versus routes versus templates) and consistency of implementation is important (see my notes on routes versus templates). Having a consistent system makes learning it easier. While I'm not a huge fan of nesting the configuration deeper, it does provide those particular benefits. Let me try it out, and we'll see how it ends up.

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

Successfully merging this pull request may close these issues.

4 participants