Bug #92397
closedRegression: New property showNewRecordLink is superfluous
100%
Description
The new property showNewRecordLink, introduced in #82489 is superfluous as the newRecordLink button can already be hidden using ['appearance']['levelLinksPosition']
Updated by Oliver Bartsch about 4 years ago
- Related to Bug #82489: No newRecordLink if config.appearance.enabledControls.new = false added
Updated by Gerrit Code Review about 4 years ago
- Status changed from New to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/65846
Updated by Gerrit Code Review about 4 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/65846
Updated by Oliver Bartsch about 4 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset d2824c9f2acb3e15cf1cd616858d2979b47ae11d.
Updated by Gerrit Code Review about 4 years ago
- Status changed from Resolved to Under Review
Patch set 1 for branch 10.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/65855
Updated by Gerrit Code Review about 4 years ago
Patch set 2 for branch 10.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/65855
Updated by Gerrit Code Review about 4 years ago
Patch set 3 for branch 10.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/65855
Updated by Oliver Bartsch about 4 years ago
- Status changed from Under Review to Resolved
Applied in changeset e97feeae1fe1bfd64850950234450c6e5bd3b41b.
Updated by Jo Hasenau over 3 years ago
- Subject changed from New property showNewRecordLink is superfluous to Regression: New property showNewRecordLink is superfluous
- Status changed from Closed to Accepted
- Priority changed from Should have to Must have
I am reopening this bug since both, the former bugfix of #82489 and the change provided with this bugfix created regressions that in the end lead to breaking changes i.e. for Gridelements users.
The original behaviour of
'enabledControls' => [
'new' => false,
]
was to disable both, the '+'-Icon of inline records AND the "New Record" link of the inline field, leaving anything else as is.
The change to "showNewRecordLink" already introduced the first breaking change, since the default was set to false, which was meant to avoid a breaking change i.e. for Gridelements users, but still it would disable the link for people, who did not make use of enabledControls before. So the link would have been gone after a core update.
With the new change, that removed the freshly introduced switch, things got worse, since the only way to get rid of the "New Record" link now is provided by
['appearance']['levelLinksPosition'] = 'none'
Of course this removes the "New Record" link, but together with it any other link will be removed too.
So users of translated records with inline fields will lose "Localize all records" and "Synchronize with original language" and other buttons too, which are in no way related to the creation of new records.
I just noticed this breaking change, because there were some issues reported on Slack or the Gitlab issue tracker, where people complained about a misbehaviour of the "New Record" link, which should not have been visible at all with a default Gridelements TCA. https://gitlab.com/coderscare/gridelements/-/issues/183
Updated by Oliver Bartsch over 3 years ago
Hi, the fact that enabledControls['new'] => false
also disabled the "New Record" link in some recent versions was a misuse of this option since it only refers to the inline control buttons, like the name suggests. See: https://docs.typo3.org/m/typo3/reference-tca/master/en-us/ColumnsConfig/Type/Inline/Properties/Appearance.html.
enabledControls (array)
Associative array with the keys ‘info’, ‘new’, ‘dragdrop’, ‘sort’, ‘hide’, ‘delete’, ‘localize’. If the accordant values are set to a boolean value (true or false), the control is shown or hidden in the header of each >record.
Since this led to misbehaviour, as reported in #82489, we removed the misuse and replaced it with a new property, which however was recognised as superfluous (due to levelLinksPosition
) a few days later. Therefore, the new property got removed again with this issue.
With the new change, that removed the freshly introduced switch, things got worse, since the only way to get rid of the "New Record" link now is provided by
['appearance']['levelLinksPosition'] = 'none'
This is true, the "New Record" link can only be disabled with levelLinksPosition => 'none'
. This however is IMO in no way a breaking change since this is the behaviour since at least v6. I've to admit, while the options' name suggest that this affects all "levelLinks", the documentation of this option is suboptimal as it only mentions the "New record" link. But as said, the behaviour did not change, only the misuse of the enabledControls
option was fixed in #82489.
Updated by Jo Hasenau over 3 years ago
The point is, that the changes before completely ignored the fact, that now ALL level links or NO level links are disabled, while the behaviour before - be it caused by a misuse or not - just disabled the "new" link, leaving the rest of the links intact.
Then again even the "fix" before was not necessary at all, since there is no logical reason, why you would want to disable the "new" link for the items in the list, while keeping it intact for the whole list via "level link". So what was considered a misuse and fixed as a bug, actually was a quite logical, useful and expected behaviour.
IMHO the problem was introduced by trying to fix something, that was not broken in the first place. In the next step, the already faulty "fix" was removed in favor of something that makes buggy behaviour even worse.
Regardless of the reasons and the classification as a feature or a bug, the change was breaking because it changed the states that are now reachable via configuration.
Before it was possible to get the "level" links responsible for translation and just remove the "new" link from those level links. Now this is not possible anymore,so people will either lose the chance to translate their Gridelements properly or they will get an unexpected "new" link, which will create broken child relations due to a lack of layout information within the container element.
By default they will get the latter, which caused several problems and bug reports in the Gridelements tracker, which can not be resolved, because the desired state of buttons and links has been completely removed by the core.
A proper fix for #82489 would have been to take care of the "maxitems" configuration and remove any "new" link as soon as the number of items has been reached, regardless of any other configuration.
Updated by Jo Hasenau over 3 years ago
Just checked the behaviour of "maxitems" in relation to the "new" button, and it already does exactly what I proposed in the last comment, regardless of the rest of the configuration.
So actually #82489 could have been "fixed" by telling the original reporter, that there is no need to touch the configuration of the "new" button at all, since "maxitems" already removes everything magically under the hood as soon as the given number has been reached.
Updated by Jo Hasenau over 3 years ago
Since it would not be desirable to revert a fix of a fix that was (IMHO) not necessary in the first place, could we agree on the following final fix to at least get back the former behaviour?
The documentation shows several switches for the "level links", which of course skipped the "new" link, since this was treated by the indirect configuration via enabledControls.
https://docs.typo3.org/m/typo3/reference-tca/master/en-us/ColumnsConfig/Type/Inline/Properties/Appearance.html?highlight=showpossiblelocalizationrecord
Using "levelLinksPosition" to disable the switch, IMHO is the same misuse as the one with enabledControls before, since this parameter is just about positioning the whole block, not about enabling or disabling single buttons or links.
So IMHO the actual fix would be to create a "showNewRecordLink" switch additional to the already existing "newRecordLinkAddTitle" and "newRecordLinkTitle".
By default it should be set to true, which would still be a breaking change for Gridelements users, but this could be fixed with a new Gridelements release then.
Updated by Oliver Bartsch over 3 years ago
- Status changed from Accepted to Closed
Hi Jo, I got your point here. We discussed this and will solve it following:
v10: Revert the whole bunch of changes, which means enabledControls['new'] => false
will still disable the "New record" link as before in v10.
v11: Remove the misuse of enabledControls['new'] => false
and add a new option showNewRecordLink
next to the already existing showAllLocalizationLink
and showSynchronizationLink
. Furthermore, since the levelLinksPosition => 'none'
can also be considerer a misuse, especially when all links can be disabled independently, we are going to deprecate this value.
This way it's also easily possible to have your extension compatible with both versions at once by adding the showNewRecordLink => false
option. This won't be interpreted in v10 but takes effect in v11.
I will therefore close this issue again and create two new issues: One for the revert an one for the change only affecting v11.
Updated by Oliver Bartsch over 3 years ago
- Related to Task #94764: Revert changes regarding newRecordLink added
Updated by Oliver Bartsch over 3 years ago
- Related to Feature #94765: Introduce new option "showNewRecordLink" added