[WIP] Twig integration #465

Closed
wants to merge 72 commits into
from

Projects

None yet

8 participants

@mpscholten
Member

This pull request is a try to improve the builder by using twig.

Plan

  • more readable code
  • decouple data and views

Future steps could be to improve handling of behaviours.

Current state:
Code:
Currently I replaced some single methods in the ObjectBuilder and the ExtensionObjectBuilder with twig templates, but maybe later we can replace the complete build-process with rendering the templates.
Also some protected methods were required to become public because the templates need to access them, I need to find a solution for this.

References:
#182

@jaugustin jaugustin and 1 other commented on an outdated diff Oct 4, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
@@ -31,6 +32,23 @@
*/
class ObjectBuilder extends AbstractObjectBuilder
{
+ private $twig;
+
+ public function __construct(Table $table)
+ {
+ parent::__construct($table);
+ $loader = new \Twig_Loader_Filesystem(__DIR__ . '/templates/');
+ $this->twig = new \Twig_Environment($loader, ['autoescape' => false, 'strict_variables' => true, 'cache' => sys_get_temp_dir() . 'propel2-cache', 'auto_reload' => true]);
jaugustin
jaugustin Oct 4, 2013 Member

sys_get_temp_dir() . 'propel2-cache' you are missing a directory separtor => travis test failing
should be
sys_get_temp_dir() . DIRECTORY_SPARATOR . 'propel2-cache'

mpscholten
mpscholten Oct 5, 2013 Member

Thanks. Fixed that.

Owner
marcj commented Oct 4, 2013

Can you explain please more, why the filter system causes problems?

The performance issue is quite interesting. Means this that the initial call (without warm cache) will always be slow? How much faster will it be with warm cache? Can you compare it to the current state with php as code generation? If we need a cache folder, we probably also need then a option in the build command to define a own path.

@jaugustin jaugustin and 1 other commented on an outdated diff Oct 5, 2013
composer.json
@@ -22,7 +22,8 @@
"symfony/finder": "~2.2",
"symfony/validator": "~2.2",
"symfony/filesystem": "~2.2",
- "psr/log": "~1.0"
+ "psr/log": "~1.0",
+ "twig/twig": "1.12"
jaugustin
jaugustin Oct 5, 2013 Member

any reason for an old version ? "~1.14" would be better :)

mpscholten
mpscholten Oct 5, 2013 Member

Thanks for pointing out 👍

Member

Good start, but I think you put to much intelligence in the twig template (e.g. _classBody.php.twig, or _columnAccessorMethods.php.twig are bads ideas, unreadable ;) ).

For me the twig template should look like a class skeleton with blocks filled in with data. Intelligence should be keep in pure php function like a twig_function or twig_filter

Example :

{% block namespace %}{% endblock %}

{% block classStart %}{% endblock %}

{% block classAttributes %}{% endblock %}

{% block classEnd %}{% endblock %}

@staabm staabm commented on an outdated diff Oct 5, 2013
src/Propel/Generator/Builder/Om/AbstractOMBuilder.php
@@ -96,6 +96,8 @@ public function build()
* This method may emit warnings for code which may cause problems
* and will throw exceptions for errors that will definitely cause
* problems.
+ *
+ * Todo: Why is this not abstract?!
staabm
staabm Oct 5, 2013 Member

Not all subclasses implement this method, therefore itnis easier to let it be a regular method. If it would be abstract all subclasses would need tonat least implement itnas empty method.

@staabm staabm commented on the diff Oct 5, 2013
...ropel/Generator/Builder/Om/ExtensionObjectBuilder.php
@@ -20,6 +22,14 @@
*/
class ExtensionObjectBuilder extends AbstractObjectBuilder
{
+ private $twig;
+ public function __construct(Table $table)
+ {
+ parent::__construct($table);
+
+ $loader = new \Twig_Loader_Filesystem(__DIR__ . '/templates/');
+ $this->twig = new \Twig_Environment($loader);
staabm
staabm Oct 5, 2013 Member

Better use DI container here?

mpscholten
mpscholten Oct 5, 2013 Member

I thought the DI container is runtime only?

hhamon
hhamon Oct 6, 2013 Member

No need to use a DIC if we just need to use a Twig object at the moment. Keep things simple and pragmatic.

Member

I think the performance with a cold cache is 4 times slower than the php-implementation. But with the cache it's not that much slower. I think the option for the cache path is a good idea, I'll look how to code this :)

Member

To the filter problems:
I think you saw I created the _classBody template which should (later) render the full class body.
It contains some includes like

{% include 'Object/_constructor.php.twig' with {'builder': builder} %}
{% include 'Object/_baseObjectMethods.php.twig' with {'builder': builder} %}

Now I have a template _attributes which should be included from the _classBody and renders the class attributes.
The problem is after the rendering of the _attributes I've to call the behaviour filters, but that's not possible from twig because I cannot give the currently renderer php-code as a reference.
Maybe you unterstand better with looking at the source:
In the _classBody I want

{%include '_attributes.php.twig'%}

But currently I have to do:

    {{ builder.addAttributes }}

which internally calls this

    /**
     * Todo: find a way to handle behaviours
     */
    public function addAttributes()
    {
        $script = $this->twig->render('Object/_attributes.php.twig', ['builder' => $this]);
        $this->applyBehaviorModifier('objectAttributes', $script, "    ");

        return $script;
    }

There's not way to implement this in twig code. I hope you understood, otherwise just ask :)

Member

@jaugustin you're right, these files are a little unreadable, but I'm note sure if this is the right way to keep so much presentation logic in the builder class. @staabm @marcj What do you think about this?

Member
staabm commented Oct 5, 2013

per se I like the idea to separate the templates from the php logic. I don't know If twig is suitable for that because I never used it myself.

Member

Now the tests are failing because I removed some builder calls like ObjectBuilder::addDefaultAccessorOpen() into a template and the i18n-behaviour is using it.
If we want to use twig, theres no way around refactoring the behaviour system. Maybe we should add a Behaviour::getAttributeTemplate(), Behaviour::getAccessorsTemplate(), ... and then in the _classBody
we could do {% for behavior in behaviours %}{% include behavior.attributeTemplate %}. WDYT?

@mpscholten mpscholten commented on an outdated diff Oct 5, 2013
...r/Behavior/I18n/I18nBehaviorObjectBuilderModifier.php
@@ -176,7 +176,7 @@ protected function addTranslatedColumnGetter(Column $column)
'functionStatement' => $functionStatement,
'columnPhpName' => $column->getPhpName(),
'params' => implode(', ', $params[0]),
- ));
+ ));*/
mpscholten
mpscholten Oct 5, 2013 Member

Currently disabled because otherwise tests cannot start

Member

Why don't use Twig Generator library? https://github.com/cedriclombardot/TwigGenerator
It was originally developed for this purpose (propel builders refactor ) and it's already tested and used (see AdminGeneratorBundle)

Member

@cristianoc72 not sure which benefits this would bring

Member

The last few days I found a, in my view, good way to manage the behavior modifiers.
Would be nice if someone can look at this and could tell his opinion on the new "api". You can currently find an example in the l18n and archivable behavior.

@jaugustin jaugustin commented on the diff Oct 9, 2013
...or/AggregateColumn/templates/Object/_methods.php.twig
@@ -0,0 +1,31 @@
+{% set query = behavior.buildQuery %}
+{% set column = behavior.column %}
+/**
+ * Computes the value of the aggregate column {{ column.name }}
+ *
+ * @param ConnectionInterface $con A connection object
+ *
+ * @return mixed The scalar result from the aggregate query
+ */
+public function compute{{ column.phpName }}(ConnectionInterface $con)
+{
+ $stmt = $con->prepare('{{ query.sql }}');
+ {% for key, binding in query.bindings %}
+ $stmt->bindValue(':p{{ key }}', $this->get{{ binding }}());
jaugustin
jaugustin Oct 9, 2013 Member

to much tab

mpscholten
mpscholten Oct 9, 2013 Member

You're correct :) My indentation is currently wrong in all these templates, I'll fix this at the end of this pr. But the good side of this is, that the code is very readable. Maybe instead of indent everything in the templates we could use a php-formatter? What's your opinion on this?

marcj
marcj Nov 4, 2013 Owner

Mh good question. The good side of a php-formatter is we can write the twig files more readable with indentation. The bad is that when we're developing new stuff into and debug it the php-formatter could create something strange when we for example have missed a } or something. That could be hard to debug then. My personal feeling is we should stay without formatter.

Member

@willdurand @jaugustin could you guys please look over the current code? Most parts of the ObjectBuilder moved to twig files now. But this process takes much time, so I want to be sure that this will be merged in the future :)

Owner
marcj commented Nov 4, 2013

Looks pretty good to me. When do you think is it done?

Member

There's pretty much code to convert to twig, so I think this will take 1 - 2 months.

tacman commented Nov 4, 2013

I'd be happy to help with that, though it'd be much easier for me if
PropelBundle were working enough that I can use it for testing. We wrote a
CRUD generator using Twig and the Propel schema, and hoped that this would
be part of 2.0. It should also make behaviors MUCH easier to write.

Tac

On Mon, Nov 4, 2013 at 2:55 PM, mpscholten notifications@github.com wrote:

There's pretty much code to convert to twig, so I think this will take 1 -
2 months.


Reply to this email directly or view it on GitHubhttps://github.com/propelorm/Propel2/pull/465#issuecomment-27717058
.

Member
staabm commented Nov 5, 2013

can we do the transition to twig in several small PRs, so we can get this landed?

Member

@tacman this would be very nice :) I added you as a collaborator in https://github.com/mpscholten/Propel2 so you can just push to the twig branch. Otherwise you can also just make a pull request, but I think it's easier for you to just push directly to it.

@staabm would be possible. We can try to get ObjectBuilder ready, and work on QueryBuilder later.

Member

Currently I'm stuck with getting the tests running again.

There was 1 error:

1) Propel\Tests\Generator\Behavior\I18n\I18nBehaviorObjectBuilderModifierTest::testPostDeleteEmulatesOnDeleteCascade
Undefined property: I18nBehaviorTest1::$bar

Problem is the i18n behavior. In the test I mentioned, the bar column won't get generated but the mutators and accesors are. Now they try to access the bar attribute like $this->bar but this attribute doesn't exists. I think this is expected behavior isn't it? (I've never used the i18n behavior myself so I don't know, but the I18NBehavior::moveI18nColumns looks like this is expected) But then the accessors and mutators need to changed to work somehow.

@staabm @tacman would be nice if you can explain the i18n behavior to me or maybe just give me a hint how I get this working again.

To reproduce the error just run bin/propel test:prepare --vendor=sqlite --dsn="sqlite:/tmp/database.sqlite" --user="test" --password="test" && phpunit -d memory_limit=1024M --stop-on-error and it should stop on the said test.

Member

ping @marcj @staabm @tacman I want to finish this pull request but I'm still stuck with the problem described before. Would be very nice if someone could help me with this.

Owner
marcj commented Jan 27, 2014

Sorry for the delay @mpscholten. Unfortunately, I've never worked with the I18n behavior. Maybe @fzaninotto can say some words on it as he has initially wrote that behavior. :)

Member

That's... a long time ago. I think Propel's i18n behavior was inspired by Symfony1 i18n behavior. I don't remember the rationale behind each microdecision. If it doesn't make sense for you, then don't hesitate to break the tests!

@gossi gossi referenced this pull request Feb 2, 2014
Open

Satisfy Behavior Authors #533

1 of 6 tasks complete
Owner
marcj commented Apr 18, 2014

@mpscholten, I guess it is still much work to do, hm? Especially to catch up with current master.

Member

Yeah that's just to much code to port over and the pull requests will end up being to huge to merge. IMO we should rethink for a better solution instead of this. In case your still interested in rewriting the builder I suggest we should use something like zend code generator together with twig and then port the code step by step.

Owner
marcj commented Apr 18, 2014

Yes, I'm sorry your work didn't pay off. I'm going to close this issue for now.

Feel free to re-open it or create a appropriate issue (for maybe other approaches), if you find the time to work on it. Otherwise we just don't address this issue anymore for now, although I'd like to use a code generator like zend's class you've pointed out. But would be just too much work at the moment.

@marcj marcj closed this Apr 18, 2014
Member

My 2c about Zend Code Generator: It sucks. Generating complex Propel
classes with this tool is horribly verbose, and we wouldn't benefit from
great Twig features (mixins, inheritance, blocks, etc).

Generating PHP code with Twig has already been pushed pretty far with
AdminGeneratorBundle (
https://github.com/symfony2admingenerator/AdmingeneratorGeneratorBundle/tree/master/Resources/templates/Propel).
I think it has some defaults, but it's way better than was we have now, and
I don't know of any better solution.

François

2014-04-18 20:26 GMT+02:00 Marc notifications@github.com:

Yeah that's just to much code to port over and the pull requests will end
up being to huge to merge. IMO we should rethink for a better solution
instead of this. In case your still interested in rewriting the builder I
suggest we should use something like zend code generatorhttp://framework.zend.com/manual/1.11/en/zend.codegenerator.examples.htmltogether with twig and then port the code step by step.


Reply to this email directly or view it on GitHubhttps://github.com/propelorm/Propel2/pull/465#issuecomment-40831962
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment