georgringer / news

TYPO3 Extension news
GNU General Public License v2.0
263 stars 357 forks source link

Plugin updater removes additional flexform fields added via hooks #2172

Closed eliashaeussler closed 11 months ago

eliashaeussler commented 1 year ago

Bug Report

Current Behavior We've extended the news flexform using a dedicated hook for FlexFormTools, registered within $GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS'][\TYPO3\CMS\Core\Configuration\FlexForm\FlexFormTools::class]['flexParsing']. This works fine and the additional fields are shown in FormEngine.

However, when running the PluginUpdater upgrade wizard, those fields get removed from the pi_flexform values. This is because of the following method: https://github.com/georgringer/news/blob/a81398060ff206bc7c7c1aff4b148e2ceadc7499/Classes/Updates/PluginUpdater.php#L185

It obviously does not include additional fields added via hooks and therefore removes all of them.

Expected behavior/output It would be nice if additional fields registered via flexform hooks can be taken into account when cleaning up flexform XMLs. This would avoid additional fields from getting removed, leading to a potential change in behavior (depending on what additional fields are configured and what impact they have).

Environment

Possible Solution I have a working solution in place which uses TYPO3\CMS\Core\Configuration\FlexForm\FlexFormTools::cleanFlexFormXML() to clean up the flexform XML. AFAICS, this method does a pretty similar job like GeorgRinger\News\Updates\PluginUpdater:: getAllowedSettingsFromFlexForm(), but additionally calls all registered hooks. I'm not sure if this solution could be used as drop-in replacement for the current implementation, therefore I'm posting it here before submitting a PR. Please let me know if this is something you'd consider useful and I'd be happy to submit a PR.

georgringer commented 1 year ago

thanks for the issue. yes please add the PR

eliashaeussler commented 1 year ago

Alright, here we go: #2173

liayn commented 11 months ago

Can confirm with eventnews