Bug #92397

Regression: New property showNewRecordLink is superfluous

Added by Oliver Bartsch over 1 year ago. Updated 5 months ago.

Status:
Closed
Priority:
Must have
Category:
FormEngine aka TCEforms
Target version:
-
Start date:
2020-09-24
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
10
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

The new property showNewRecordLink, introduced in #82489 is superfluous as the newRecordLink button can already be hidden using ['appearance']['levelLinksPosition']


Related issues

Related to TYPO3 Core - Bug #82489: No newRecordLink if config.appearance.enabledControls.new = falseClosed2017-09-15

Actions
Related to TYPO3 Core - Task #94764: Revert changes regarding newRecordLinkClosedOliver Bartsch2021-08-09

Actions
Related to TYPO3 Core - Feature #94765: Introduce new option "showNewRecordLink"ClosedOliver Bartsch2021-08-09

Actions
#1

Updated by Oliver Bartsch over 1 year ago

  • Related to Bug #82489: No newRecordLink if config.appearance.enabledControls.new = false added
#2

Updated by Gerrit Code Review over 1 year 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

#3

Updated by Gerrit Code Review over 1 year 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

#4

Updated by Oliver Bartsch over 1 year ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
#5

Updated by Gerrit Code Review over 1 year 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

#6

Updated by Gerrit Code Review over 1 year 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

#7

Updated by Gerrit Code Review over 1 year 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

#8

Updated by Oliver Bartsch over 1 year ago

  • Status changed from Under Review to Resolved
#9

Updated by Benni Mack over 1 year ago

  • Status changed from Resolved to Closed
#10

Updated by Jo Hasenau 6 months 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

#11

Updated by Oliver Bartsch 5 months 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.

#12

Updated by Jo Hasenau 5 months 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.

#13

Updated by Jo Hasenau 5 months 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.

#14

Updated by Jo Hasenau 5 months 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.

#15

Updated by Oliver Bartsch 5 months 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.

#16

Updated by Oliver Bartsch 5 months ago

  • Related to Task #94764: Revert changes regarding newRecordLink added
#17

Updated by Oliver Bartsch 5 months ago

  • Related to Feature #94765: Introduce new option "showNewRecordLink" added

Also available in: Atom PDF