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

Adding twig template engine #28

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

othmar52
Copy link
Contributor

Hi Artur,
What do you think about adding a template engine to get rid of the code mashup (php/html/js/css within single files)? twig is a powerful template engine and is dead easy to use.

Instead of building horrible strings with php now you can simply call

echo $twig->render('path/template.htm', $varsNeededInView);

i already included an example in this pull request for index.php?action=viewRandomAlbum
@see https://github.com/othmar52/OMPD/blob/twig/controller/viewRandomAlbum.php
@see https://github.com/othmar52/OMPD/blob/twig/templates/randomAlbum.htm
@see https://github.com/othmar52/OMPD/blob/twig/templates/partials/albumTile.htm

further i have splitted the unmaintainable file index.php which had more than 3000 lines of mashup into multiple files @see directory /controller/

instead of processing the whole music collection within on mysql-fetch-loop smaller
batchsizes gets processed. now there is NO(!!!) memory growth within importer-phase
fileInfo() at all.
Unfortunately the memory-usage of the previous importer-phase (fileStructure) is
still a problematic starting point for fileInfo()

further some massive code-cleanup regarding tag-processing had been done.
TODO: make use of the new tag-processing where needed to remove duplicate code-mess
TODO: refactoring fileStructure() in terms of memory consumption
to get rid of the code-mashup of php/html/js/css/mysql within single files
a powerful lightweight template-engine had been added via composer.

further the mess of index.php had been splitted into smaller files to /controller
a lot of O!MPD's javascript is build with php because of php variables are
directly echoed into js variables. Now we have an additional route
/index.php?action=jsConf for retrieving dynamic variables.

so inside javascript those variables are easy usable via
window.ompd.varName

the twig template for this config object is located in
templates/js/ompd-conf.js
@othmar52
Copy link
Contributor Author

Hi Artur,
Regardless of those merge conflicts - what is your opinion about having twig template engine in O!MPD?
do you like the idea?

@ArturSierzant
Copy link
Owner

I've been thinking a lot about this (and composer too) and I have mixed feelings... From one hand the idea itself is OK and it'd be fine to built O!MPD using modern programming techniques. From the another hand I don't really see its significant advantages in this project. Rebuilding all code to use twig will take a lot of time and probably generate new problems. Time spent on this could be used for something else, eg. improving update procedure or adding new features (which is more enjoyable for me :)).

I think that O!MPD should be somehow consistent - if it uses twig, it should use it everywhere, in all pages.
The question is: how much time would it take to rebuild O!MPD to use twig that way? And is it possible at all?

Maybe it'd be better to tidy and clean up the code first - splitting index.php into separate files is good move, I think.

I really appreciate your work and time you spent on making O!MPD better, but for now I'm not convinced to this idea.

@othmar52
Copy link
Contributor Author

Hi Artur,

...Rebuilding all code to use twig will take a lot of time ...
...I think that O!MPD should be somehow consistent ...

You are totally right that it makes no sense to roll O!MPD's code from bottom to top. But you really prefer to have (and produce more) unmaintainable code just to have it consistent? This sounds kinda strange

Adding a template engine is a pretty tiny change that can reduce line-count of all the php functions from 10% up to 90%. From my point of view maintainability is far more important than consistency.

... if it uses twig, it should use it everywhere...

Of course this would be more elegant but there is not expicitly a technical need for it. Implementing a template based rendering can be limited to sections that have to be touched when developing new features or fixing bugs. The existing archticture/logic of O!MPD does not get changed by adding a template engine.

From the another hand I don't really see its significant advantages in this project.

So getting rid of highly complex code parts that are very fragile is not a significant advantage for you? Seriously? I thought your plan was to continue your work on O!MPD in future!?

...and probably generate new problems.

From my point of view its the opposite. Tons of code-duplication can be removed by proper reusing existing html snippets.

...Maybe it'd be better to tidy and clean up the code first ...

You are totally right. Tidy things up without having any changes is exactly what this pull request does

I really appreciate your work and time you spent on making O!MPD better,
but for now I'm not convinced to this idea.

It's not the decision i wanted to hear but i am totally ok with it - O!MPD is your baby

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.

2 participants