-
Notifications
You must be signed in to change notification settings - Fork 73
[BUGFIX] hide container in wizard if flag is set in registry #633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUGFIX] hide container in wizard if flag is set in registry #633
Conversation
904e2ac to
43adcce
Compare
|
this wont work. fails on multipe containers... will look into it tomorrow or somebody can take it over |
Classes/Tca/Registry.php
Outdated
| return $pageTs; | ||
| if ($typo3Version->getMajorVersion() >= 13 && $containerConfiguration['registerInNewContentElementWizard'] === false) { | ||
| $content .= "mod.wizards.newContentElement.wizardItems.container.removeItems := addToList({$containerConfiguration["cType"]})"; | ||
| return $pageTs . LF . $content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this return statement shouldn't be in foreach loop, but after it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're absolutely right. also saw it! also left a comment and changed the pr description. thank you
|
thanks for PR, i think code should be like this: (i can care about it, and i think we have to fix also the tt_content_defValues from |
|
@achimfritz for multiple containers it needs to be a bit different i think. it only unsets the first item. might be that this is not a valid syntax and that it needs to be addToList(1col-container, 2col-container) if you want i could take a look tonight when i'm home. could be that i was just tired and saw things that weren't there whhahah. could give you a definite answer tonight |
ceb5d7a to
2814888
Compare
|
@achimfritz @chesio Thank you both for checking the PR. I fixed the hardcoding of but also i think i went for a okay approach in the PR. I explicitly didn't squash for the reviewing not being a mess in github. if its okay please let me know i wil squash it before merging. You could also take it as a base. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after that i will be happy with your PR and it will be nice, if you squash the commit.
Thanks again.
Classes/Tca/Registry.php
Outdated
| $defaultGroup = 'container'; | ||
|
|
||
| $typo3Version = GeneralUtility::makeInstance(Typo3Version::class); | ||
| $cTypesByGroup = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would prefer to give a more readable name for the variable, e.g. $cTypesExcludedInNewContentElementWizard
|
|
||
| if ($typo3Version->getMajorVersion() > 12 && !empty($cTypesByGroup)) { | ||
| foreach ($cTypesByGroup as $group => $ctypes) { | ||
| $pageTs .= LF . 'mod.wizards.newContentElement.wizardItems.' . $group . '.removeItems := addToList(' . implode(',', $ctypes) . ')'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have to increase the count of the ignored errors in each Build/phpstan-baseline-1<1-3>.neon to make CI happy
e.g.
+++ b/Build/phpstan-baseline-13.neon
@@ -3,7 +3,7 @@ parameters:
-
message: "#^Constant LF not found\\.$#"
- count: 4
+ count: 5
path: ../Classes/Tca/Registry.php
|
Hey! i had a busy weekend. will fix it tonight. Sharp of you too see the naming convention. I agree with changing it, also with bumping up the count of the error in de stan files. I will make a loose commit to easily see the change. If all is good i will squash it. |
|
@achimfritz pushed the renaming and error in a loose commit ready for review. Let me know if it's all good. Then i will squash it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…lementWizard' is set to false.
af7243d to
9438301
Compare
|
@achimfritz everything is squashed. Commit name is also fitting? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Description
Currently when setting the flag
->setRegisterInNewContentElementWizard(false)nothing happens in v13.this is happening because the pageTS is returned early before it is build. the code in action
A hint was already given in the comment namely the changelog.
https://docs.typo3.org/c/typo3/cms-core/main/en-us/Changelog/13.0/Breaking-102834-RemoveItemsFromNewContentElementWizard.html
The goal
respecting the setting given by the TCA override `->setRegisterInNewContentElementWizard(false)``
Scope
13.4
understanding the change
Before we return the pageTs early we first check if there is a container that needs to be unset. If so the early returned pageTS will now contain the logic to remove the containers we don't want to see.
links
extra information
Tested it in a ddev v12 environment and ddev v13 environment.
tested
✅ single container
✅ multipe containers