search

fnagel / beautyofcode

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

Check TYPO3 7.5 compatibility #10

Closed dreadwarrior closed 8 years ago

dreadwarrior commented 9 years ago

It seems the extension is not working properly with TYPO3 7.5.

fnagel commented 9 years ago

Frontend rendering seems to work. BE element throws following error on edit:

1: PHP Catchable Fatal Error: Argument 2 passed to TYPO3\Beautyofcode\Configuration\Flexform\T3EditorWizard::main() must be an instance of TYPO3\CMS\Backend\Form\FormEngine, instance of TYPO3\CMS\Backend\Form\Element\TextElement given in /var/www/xyz/typo3conf/ext/beautyofcode/Classes/Configuration/Flexform/T3EditorWizard.php line 203 (More information)
uklimaschewski commented 9 years ago

We are experiencing the same here, TYPO3 7.5.0

dreadwarrior commented 9 years ago

Hey Felix,

Further issues found so far:

So I'd like to know how to proceed with this? For me it looks like we should loose BC or otherwise add a lot of hacks and if/else-statements while upgrading to 7.5).

Best,

tommy

dreadwarrior commented 9 years ago

Here are some first fixes for some of the issues:

https://github.com/dreadwarrior/beautyofcode/commit/0f66cc46254fffbff2340b886ed1f417b198a7bb

fnagel commented 9 years ago

Hey Tommy, thanks for taking a look!

Not sure how to proceed. So, let me try to sum this up: autoloading issues sound fixable, right? t3editor sounds like something I could try to fix somehow, perhaps it's even a CSS issue only. Brush / language loading sounds like the big deal here, right? What exactly needs to be adjusted?

For me it looks like we should loose BC or otherwise add a lot of hacks and if/else-statements while upgrading to 7.5

Can you explain this further? Are you talking of a 7.5 only version?

dreadwarrior commented 9 years ago

Hi Felix,

yes autoloading is maybe fixable. But I could not get it working with Helmut's blog articles. Neither updating ext_emconf nor introducing a composer.json and dumping the autoload information with composer after the adjustments result in proper class loading by T3.

Brush/language loading is based on loading TypoScript configuration from the backend context. The hook for the flexform userFunc does not seem to get the record PID anymore. But this is necessary in order to properly load the TypoScript configuration via the TemplateService.

The Brush loading re-work I've done some months ago would make boc independent of the API but I think this is not easily to merge now as the focus must be on the 7.5 compat. But we should try to get this feature asap into the boc core.

With the fixes in the previous comment the brush / language loading is working at least for existing plugin CEs. But not for new ones. The current method to retrieve the Page UID in this scenario is not optimal IMHO.

Either we provide a 7.5+ only version or have to introduce "feature flags" / T3 version checks in order to load other classes which act responsible for a specific version.

What do you think?

dreadwarrior commented 9 years ago

I updated the branch "fix_10" in my boc fork with adjustments regarding

The latter one is still not working properly: the t3editor is collapsed in width. My JS skills are not that good to fix that one. It seems our workaround from older versions are not working currently. To give more context: this workaround was necessary because t3editor dimension calculation doesn't seem to work properly in flexform contexts (maybe because the area is hidden on load).

Would be great if you can have a look :)

Bless!

tommy

fnagel commented 9 years ago

yes autoloading is maybe fixable. But I could not get it working with Helmut's blog articles. Neither updating ext_emconf nor introducing a composer.json and dumping the autoload information with composer after the adjustments result in proper class loading by T3.

What issues are caused by this? Seems to work nice. Did you try clearing cache the hard way (truncate in DB, removing typo3temp/Cache/*)? Uninstall / Install extension? Class loading and Caching in newer TYPO3 versions can be very frustrating.

dreadwarrior commented 9 years ago

Well, I was not able to use boc in the backend at all. T3 failed while loading the FlexformT3editor for example. Didn't tried clearing database cache nor re-installing boc but clearing typo3temp/Cache/ + via Install Tool "Clear all caches".

I'm glad it works for you. Thought I f'ed up my setup. I worked around this by adding a psr-4 autoload setting to my root composer.json and dumped the autoloading information.

I hope class loading is more usable since 7.5 because it's based on composer. But maybe some old Flow relics interfer with the new, better approach ;) Caches ... oh yes T3 has too much of 'em! ;)

fnagel commented 9 years ago

Summery of our call:

New branch: https://github.com/fnagel/beautyofcode/tree/TYPO3-7.5

fnagel commented 9 years ago

TS generation issue in BE hook

See 96cdee995cafe6412884b2f571a178bb846dbd8f

fnagel commented 9 years ago

Proposed change: 72874875238a3fafc4942eb4d50ef7cf7cce2814

Using settings service for getting TS setup. More default / core way adopted by EXT:news.

dreadwarrior commented 9 years ago

@fnagel Just wanted to let you know: the fix is on its way. Had to spend some big effort to prepare my website for a TYPO3 7.5. update (Removal of all FluidTYPO3 extensions). This was necessary as I thought it would be a good idea to let boc work within an actual project environment instead of a vanilla installation.

dreadwarrior commented 9 years ago

Uff... It's done. The extension now is using a custom CType. Please check and review the TYPO3-7.5 branch. I made a second commit with some codestyle improvements.

fnagel commented 9 years ago

Nice and pretty big one! Migration works fine but I noticed some issues:

ps: I must confess I don't really like the copyright header change in latest commit. Other changes are fine...

pps: We should create a PR and discuss on the actual code.

dreadwarrior commented 9 years ago

Hey Felix!

Thanks for your valuable feedback!

It seems the inheritence for prism languages is broken.

This is currently not supported, but subject to change in an upcoming version with the dynamic brush loading. The prism dependency "problem" and how to configure properly is documented here

Question: we do add JS files per page dynamically based upon the used languages, right?

Yes. The brushes configured via TypoScript are traversed and applied to the PageRenderer. I think in the latest T3 version the files are loaded - no matter if they're existing.

I think we need to improve the ViewHelpers to check for file existence.

[...] the bodytext field could be renamed in code or something similar

This is a no-brainer and can be easily accomplished by adjusting the TCA.

NewContentElementWizard does not work any more

Any detailed error messages or exception strack traces for this issue? I'll have a look anyway... ;)

Do you have any ideas regarding the file doc headers? I applied the current CGLs for the change. The CodeSniffer and typo3-ci CS-rules don't allow any other content in the copyright notice.

I added a PR

fnagel commented 9 years ago

[...] the bodytext field could be renamed in code or something similar

This is a no-brainer and can be easily accomplished by adjusting the TCA.

I'm not sure if this can be accomplished for a single tt_content element ctype only.

NewContentElementWizard does not work any more

No errors but, it's trying to add beautyofcode_contentrenderer as a plugin. Added a fix for that.

It seems the inheritence for prism languages is broken.

This is currently not supported [...] The prism dependency "problem" and how to configure properly is documented here

I did not change any TS configuration and it broke after the upgrade so I expected a bug. But it actually was an old additional template in my test setup. Just forget about this one :-/

I think we need to improve the ViewHelpers to check for file existence.

I've added this fix, works nice so far.

fnagel commented 9 years ago

Got a patch file (for 7.5) via Slack: fix_after_formengine_changes.txt

Seems like a similar fix you tried to implement. This will not work with 6.2, right?

also idee 1. den typehint auf die FormEngine raus nehmen. dann sollte es in 6.2 und 7 funktionieren idee 2. das $row[$field] mit einem is_array check verstehen und wenn kein array dann xml2array drum packen das sollte die beiden von mir gefixen punkte für beide versionen kompatibel machen neues element habe ich noch nicht getestet. autoloading war kein problem. sowohl backend als auch frontend funktionieren bei mir

fnagel commented 9 years ago

@dreadwarrior Added some more commits: Renamed bodytext and moved it to plugin tab (like in old version), fixed the flexform localization issue (both configuration panels visible).

dreadwarrior commented 9 years ago

@fnagel Sorry for not replying so long. This looks really great!

Will the t3editor (re-)size properly in the plugin tab? I thought the hiding of it in CE tabs will break the sizing, but maybe the core implementation is better in native TCA but flexform...

The suggested patch from slack will definitely break BC for 6.*. So removing the typehint and doing some instanceof-checks is the way to go or we just create a new major version with incompatibility set to < 7.5. What do you think?

fnagel commented 9 years ago

Will the t3editor (re-)size properly in the plugin tab?

No, you are right. Still an issue. Thing is, its the same issue when switching to the General tab. Just the other way around (opening CE on Plugin tab will break the t3editor field in General tab). So this is an issue no matter what we do, right? :-/

Regarding the next release: What are our options?

Anything more to consider? What would you say we should do?

fnagel commented 8 years ago

Did some tests in TYPO3 7.6 - works nice! Good thing is: it seems the collapsed t3editor issue is fixed in 7.6! Moved the bodytext field to the plugin tab again and was not able to reproduce the issue.

fnagel commented 8 years ago

I've added a proposed change to re-implement dynamic t3editor format configuration. Let me know what you think!

dreadwarrior commented 8 years ago

Again my apologies for these long delays during my replies. Testing in 7.6 will take some time. Have too much to do currently :/

I think the best approach will be to release a 7.x version only. The latest changes show what is inevitable: BC due of TYPO3 core API changes.

I reviewed your changes under 7.5. The collapsing editor is still an issue here but ok for me. Mode highlighting for the backend view is not working. Don't know if its because of 7.6.

I updated the T3editorElement with some more brush-to-mode map entries.

fnagel commented 8 years ago

Yeah, no problem Tommy! Know that feeling :-D

Ok, thanks for your assessment. I guess another point is that if we release a 6.2 compatibly version with the current code base would remove features from a perfectly fine current TER release (at least t3editor). Guess we'll go with the 7.x only release.

I have no working 7.5 installation at hand but looking into the TYPO3 source it should work. Regarding the collapse issue: pretty sure this is not fixed in 7.5 but it seems like it's working for 7.6

These brushed have the same keys, so that worked before, but I guess your changes make it more fail safe.

So, what needs to be done for our upcoming release? Any tasks you can think of? Some more code we could remove when dropping 6.2 support? Documentation? Tests?

fnagel commented 8 years ago

A gentle reminder... :-)

dreadwarrior commented 8 years ago

Yes, I think we need to test the changes introduced with the T3editorElement. See the newest commits in the TYPO3-7.5 branch in my fork which is currently under heavy WIP. (too much API changes in t3 core and test API - UnitTestCase is new in 7.6., isn't it?)

I'm fine with the documentation. The update procedure is clearly outlined and the t3editor was the big show stopper here.

fnagel commented 8 years ago

Yes, I think we need to test the changes introduced with the T3editorElement.

Please give me some instructions what needs to be tested. Using latest 7.5 branch in my live website and it seems to work without issues.

Uhh I see. Looks nasty. Perhaps this file is helpful? Taken from the Fluidext Extensions. https://github.com/fnagel/t3extblog/blob/master/Tests/bootstrap.php

dreadwarrior commented 8 years ago

Thanks Felix! Will try to integrate that one as soon as possible. I like to finish this soon after that long time ;)

fnagel commented 8 years ago

Any news on this issue? Should I try to fix the tests?

dreadwarrior commented 8 years ago

Yes, please. Didn't find time these days... Too much to do in office and xmas preparations... Sorry again!!!

fnagel commented 8 years ago

OK added a commit with working unit tests. Needed to skip quite some tests as these would need some more fixes adjustment due to latest changes. Hope you are are able to take over from here...

fnagel commented 8 years ago

@dreadwarrior Any chance you will find some time to adjust the tests? Wish you a successful new year!

dreadwarrior commented 8 years ago

Hi Felix,

thanks for the reminder. All the best for you, too! I've fixed the tests now.

During development, I came to the conclusion, that the following tests and the SUT needs serious refactoring:

Tests\Unit\Configuration\Flexform\LanguageItems\SortedAppendingTest Tests\Unit\Configuration\Flexform\LanguageItems\UniqueAppendingTest

Unfortunately, I removed ext:beautyofcode as a dependency of my website projects. So it will be hard for me to give any support in the future.

If the test suite and the compatibility of ext:beautyofcode with the latest TYPO3 releases is recovered and finished / this issue is closed, I have to admit my withdrawal of this project.

Thanks for all your patience, the precious code reviews and your engagement in this project.

All the best,

tommy

fnagel commented 8 years ago

Hey Tommy,

sad to hear you are no longer able to help out on this project! Especially when some tests still won't work -- which is a blocker for the new release. Can you give me some hint what needs to be done for the AppendingTests? Tried to combine them into one test but without success. any hint?

Anyway, thanks for all your help and contribution for this project. Hope you had a good time!

Yours Felix

dreadwarrior commented 8 years ago

Hi Felix,

I think the biggest problem is this line. This approach is mentioned in a comment on php.net, but it leads to problems during the unit tests.

Due this part of the code was inherited from version 1.0 of boc I don't have precise knowledge about this, but think it was introduced to reduce processing when editing multiple plugins / content elements.

I think the best solution would be to use the TYPO3.CMS Caching Framework with the TransientMemoryBackend. So we need to

What do you think about this?

Kind regards,

tommy

fnagel commented 8 years ago

Hey Tommy, thanks for the feedback! Really helpful!

As this was one of my first (the first?) extensions, I doubt this line is really needed. Probably just copied from some existing extension. Shame on me :-/

Your proposal sounds good, but I'm not sure if its really needed. Do you think we need that sophisticated caching for a simple BE method? After all this functionality is only called on opening a BoC CE in backend, right?

Anyway, using a TransientMemoryBackend sounds not that bad -- at least this does not need any extra DB or similar. One question:

This becomes handy if code logic needs to do expensive calculations or must look up identical information from a database over and over again during its execution.

https://docs.typo3.org/typo3cms/CoreApiReference/CachingFramework/FrontendsBackends/Index.html#transient-memory-backend

Sounds like this won't help in our case as it's only called once for each execution afaik.

dreadwarrior commented 8 years ago

Hi Felix,

well, you don't want to have a look into one of my first extensions ;) Don't worry, we'll find a clean solution how to get rid of this historic feature :)

I think the static variable comes in handy only if a backend user is using the batch editing capabilities of TYPO3 (List module -> Use clipboard #2 -> Select multiple content element checkboxes -> Click on the editing icon). If multiple boc content elements are edited, the processing of TypoScript, Flexform postprocessing etc. pp. will probably be expensive, because if no static variable or cache is used, the whole thing is executed in each flexform / content element context over and over again. Then, the TransientMemory will be a better solution than the current static variable.

But: I also think this is really an edge case. Experience shows that TYPO3 editors will not use this power feature very often.

Therefore, I think we can drop the static variable and the unit tests should be working like a charm, even if executed in parallel.

fnagel commented 8 years ago

Hey Tommy,

I've implemented the memory backend -- one could say for research purposes :-D Works like a charm and the tests are fully functional again!

I will do some more testing, update Prism vendor and release the new version soon.

Thanks again for all your help! Really hope this is not the last time we built something together. I really enjoyed working with you!

Yours Feliy

dreadwarrior commented 8 years ago

Hi Felix,

great to read! I had a look at the commits you made. Looks good and very self-describing. Much better than the hidden static var. Good job!

tommy

dreadwarrior commented 8 years ago

@fnagel Just saw, that you've released 3.0.0 today. Good job! May I close this issue then?

fnagel commented 8 years ago

Yes, you may close this. Thanks again for all your help!

dreadwarrior commented 8 years ago

You're welcome. Had a great time. All the best! :)