fnagel / beautyofcode

TYPO3 CMS Extension beautyofcode
https://extensions.typo3.org/extension/beautyofcode/
GNU General Public License v2.0
6 stars 8 forks source link

Extbase refactoring #2

Closed dreadwarrior closed 10 years ago

dreadwarrior commented 10 years ago

Hi Felix,

I spend the last days with a massive refactoring of ext:beautyofcode to make it TYPO3 6.+ compatible regarding extbase/fluid and namespace usage.

I know, this is a huge change, but I wanted to let you know and kindly ask you to have a look at this and tell me if you're interested in a merge.

Thanks and happy weekend,

tommy

fnagel commented 10 years ago

Hey Tommy,

I'm pretty busy atm but I took a quick look and everything looks very promising! Thanks for all the hard work and fixing all these CGL issues.

Afaik beautyofcode should be compatible at least with 6.1 but a refactoring to extbase was always on my todo list for version 2.0.

Some thinks I noticed when I reviewed the code and some general ideas:

Are you interested in becoming a beatyofcode team member on forge? This would include push right for the GitHub repo too. I would add a 1.x branch so we can work on master until v2.0 is released.

Thanks for your contribution. You did a great job!

Regards Felix

dreadwarrior commented 10 years ago

Hi Felix,

thanks for your review, your notices & your kind words.

Yes beautyofcode is compatible with 6.1, but I thought it would be a good idea to refactor into MVC decoupling and this for, to make usage of the DI, Autoloader and all the other nice stuff which comes with Extbase.

I highly appreciate & accept your invitation to become a team member on forge & GitHub push access.

Please let us clarify the namespacing/vendor things first before you open the repo for me.

dreadwarrior commented 10 years ago

Addendum regarding namespaces: I think we can use TYPO3\Beautyofcode as a namespace, because the wiki article only states that \TYPO3\CMS shouldn't be used by extension developers.

What do you think about it?

Addendum regarding caching: different configured plugins (in terms of "gutter", "line highlighting", etc..) I don't need to bother because the config goes into the class-attribute of the SH wrapper. So caching the generated inline JS code is a good idea to save processing times, I think.

dreadwarrior commented 10 years ago

A little update about the progress for backwards compatibility (TYPO3 v4):

https://github.com/dreadwarrior/beautyofcode/compare/extbase_refactoring...extbase_refactoring_T34

Things done so far:

Next up:

fnagel commented 10 years ago

Hey Tommy,

I agree, version 2.0 should make use of "modern" coding paradigms, so I'm totally ok with that. Doing so breaks TYPO3 version compatibility to 4.x so this is basically the same discussion. I fine with supporting 6.x only in version 2.0. Just a few weeks until 6.2 LTS is released which should include a migration path for 4.x projects. Lets keep "real" namespaces and make use of all the nice new features of 6.x. Supporting 4.x and 6.x is a pain in general anyway ;-)

I agree, using settings TS seems reasonable and straight forward when switchting to Extbase. Let's do that. Using VHs for exploding, list expansion, etc. sounds good to me too.

Mhh, I would not like to add another template engine and therefore a dependency. What about just using one simple FLUID template for HTML and JS? This would only need a simple viewhelper which passed the JS to the pageRenderer.

I've introduced the inline JS caching to save Fluid template processing cycles. Maybe this is an issue with different configured plugin instances, right?

I was just curious why you did it? Are those operations that expensive?

So caching the generated inline JS code is a good idea to save processing times, I think.

I never experienced any performance issues but as you already implemented it we surely should keep it!

The less 3rd party libraries, the less we have to deal with different configurations and (atm) VersionAssetServices.

I totally agree with that!

TYPO3\Beautyofcode sounds good to me. Let's use that for now. So adding you to the repo now, right? You're already added at forge.

I've added a 1.x branch as promised, so lets keep working on master https://github.com/fnagel/beautyofcode/tree/v1.x

Shall we switch to GitHub issues or stay at forge for bug / feature tracking? What do you think?

dreadwarrior commented 10 years ago

Hi Felix,

ok so we drop T3v4 compat completely. That's ok.

In the meantime I've made a few tests and it's possible to introduce a domain object for flexform settings handling via using the Extbase DataMapper. The only disadvantage is to specify a (stub) TCA definition for the flexform fields. Please see the following commits:

https://github.com/dreadwarrior/beautyofcode/commit/c21a42e27f94e144d89cb963a4e74433969a2976 https://github.com/dreadwarrior/beautyofcode/commit/989b9344b8b680511aae87cabf6983a4949dcb13

But, using the settings. prefix for the flexform values is far more lightweight! So I prefer using this approach.

What about just using one simple FLUID template for HTML and JS? This would only need a simple viewhelper which passed the JS to the pageRenderer.

Unfortunately I don't completely understand how to do this. In terms of the separation of concerns paradigm I think it's better to not mix HTML and JavaScript output. The problem is: there are too many configuration options which lead to a different output of the inline JS for different configurations. As Fluid is the up-to-date template engine in TYPO3, I thought about using it which previously was a big string, build together with PHP. BUT: we can surely use a way simpler template system for the JS inline templates. My idea is still a basic PHP view which will not introduce another dependency and will probably lead to more readable JS template code.

So adding you to the repo now, right? You're already added at forge.

Yes, thank you very much.

I've added a 1.x branch as promised, so lets keep working on master https://github.com/fnagel/beautyofcode/tree/v1.x

Great! Thanks again!

Shall we switch to GitHub issues or stay at forge for bug / feature tracking? What do you think?

I would stick to forge as an issue tracker, but GitHub for PRs etc.

Best regards,

tommy

fnagel commented 10 years ago

Hey Thommy,

my intent was to pass the whole flexform object into the template so its easier to have easy access to the getter methods. We need the stub TCA to make extbase work with the model right?

I think I just really understand what you mean by using the .settings approach: having a flexform setting. configuration maching the settings. in TS which is merged before using, right?

We could use another template for rendering JS but still using default FLUDI rendering with sections and / or some VH for exploding etc. We would need a extra VH to add our template JS as inline JS to the pageRenderer.

What about a hangout / skype call next few days?

dreadwarrior commented 10 years ago

Hi Felix,

yes, currently there need to be some TCA definitions in order to make usage of the datamapper functionality and to create a proper domain object instance. The usage is simple: just pass in an associative array of lower_underscored key/value pairs into the datamapper map method.

Thus, it's really easy to make usage of domain object getter methods within the Fluid template. I think this is the best way. The flexform, it's a configuration, but it's also a kind of domain object (expand values, etc.).

But, your suggestion to make a hangout/skype is great! What about saturday at 4p.m.?

Thanks for adding me to the repo!

Best regards,

tommy

fnagel commented 10 years ago

Hey Tommy,

saturday 4pm sounds great. Let's discuss the details then.

Regards Felix