Skip to content
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.

Sign up for GitHub

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

Add option to skip saving composite fields #229

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Add option to skip saving composite fields #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfe8f2b
[FEATURE] Add option to skip saving composite fields
Sep 4, 2024
b2ae173
[TAKS] cs fix
Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions Classes/Domain/Form/Finishers/LoggerFinisher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class LoggerFinisher extends AbstractFinisher implements LoggerAwareInterface
*/
protected $defaultOptions = [
'finisherVariables' => [],
'includeHiddenElements' => true,
'skipElementsTypes' => 'ContentElement,StaticText',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to turn this into a list of strings aka array instead?

];

/**
Expand Down Expand Up @@ -92,8 +94,32 @@ protected function getFormValues(): array
{
$normalizedFormValues = [];
$formDefinition = $this->finisherContext->getFormRuntime()->getFormDefinition();
$skipHiddenElements = !$this->parseOption('includeHiddenElements');
$skipElementTypes = GeneralUtility::trimExplode(',', $this->parseOption('skipElementsTypes'));

foreach ($this->finisherContext->getFormValues() as $identifier => $formValue) {
$element = $formDefinition->getElementByIdentifier($identifier);
$elementType = null;
if ($element !== null) {
$renderingOptions = $element->getRenderingOptions();
$elementType = $element->getType();
}
if (
$skipHiddenElements &&
(
// Mimik the logik of \TYPO3\CMS\Form\Domain\Finishers\EmailFinisher
// with conditions against {formValue.value} and {formValue.isSection} in
// EXT:form/Resources/Private/Frontend/Templates/Finishers/Email/Default.html
// where {formValue.isSection} is set in \TYPO3\CMS\Form\ViewHelpers\RenderFormValueViewHelper::renderStatic()
Comment on lines +110 to +113
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment could be added in the commit description, I'd rather not have it here.

($renderingOptions['_isCompositeFormElement'] ?? false)
|| ($renderingOptions['_isSection'] ?? false)
// additionally skip configurable element types which don't actually have form values (e.g. StaticText)
|| in_array($elementType, $skipElementTypes)
)
) {
continue;
}

if (is_object($formValue)) {
if ($formValue instanceof ExtbaseFileReference) {
$formValue = $formValue->getOriginalResource();
Expand All @@ -108,8 +134,6 @@ protected function getFormValues(): array
continue;
}

$element = $formDefinition->getElementByIdentifier($identifier);

if ($element instanceof StringableFormElementInterface) {
$normalizedFormValues[$identifier] = $element->valueToString($formValue);
continue;
Expand Down
9 changes: 9 additions & 0 deletions Configuration/Form/Setup.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ TYPO3:
identifier: header
templateName: Inspector-CollectionElementHeaderEditor
label: formEditor.elements.Form.editor.finishers.LogFormData.label
200:
identifier: 'includeHiddenElements'
templateName: 'Inspector-CheckboxEditor'
label: 'formEditor.elements.Form.editor.finishers.LogFormData.includeHiddenElements.label'
propertyPath: 'options.includeHiddenElements'
9999:
identifier: removeButton
templateName: Inspector-RemoveElementEditor
Expand All @@ -32,3 +37,7 @@ TYPO3:
formEditor:
iconIdentifier: form-finisher
label: formEditor.elements.Form.editor.finishers.LogFormData.label
predefinedDefaults:
options:
includeHiddenElements: true
skipElementsTypes: 'ContentElement,StaticText'
12 changes: 12 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@ finishers:

The `LogFormData` finisher should be the last finisher or right before the `Redirect` finisher if used. Logging after a redirect is not possible.

## Configuration

### Save values from hidden or composite Elements

Some form elements, such as fieldsets or other containers, do not hold a form value by themselves; only their child elements hold values. By default, all form elements are saved, even those that never contain values, which can result in empty columns in the log records and exports.

To prevent this, set the finisher option `includeHiddenElements` to `false`. This option ensures that values from composite elements are not saved, similar to how the `EmailFinisher` only includes fields in emails that actually contain a value.

Additionally, you can use the finisher option `skipElementsTypes` to exclude specific form elements (comma separated list). By default, this option excludes `ContentElement` and `StaticText` elements, as they will never have a value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the PR, this feature is nice but should be done separately.


### Logging of finisher variables

Additional variables stored in the `FinisherVariableProvider` can also be logged by using the `finisherVariables` option:

```
Expand Down
4 changes: 3 additions & 1 deletion Resources/Private/Language/Database.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
<trans-unit id="formEditor.elements.Form.editor.finishers.LogFormData.label">
<source>Log form data</source>
</trans-unit>

<trans-unit id="formEditor.elements.Form.editor.finishers.LogFormData.includeHiddenElements.label">
<source>Store composite form element data?</source>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not match the "hidden" naming of the feature. Also question marks are not necessary for checkbox labels.

Finally I'd suggest this:

Log hidden form elements

Copy link
Author

@johfeu johfeu Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbrodala I am not sure this is the right naming as its not about hidden elements but more about skipping functional elements. The term hidden came from the original PR as there used to be hidden elements in earelier versions.
The ones we want to skip are not hidden, just wouldn't hold a values if you think of grid containers or fieldsets. In my opinion there would be no option at all but the logger would simply skip those as the EmailFinisher is doing. I can't see no use case where you want those to be logged at all.
So if you still want that behaviour or to keep it as default, i would suggest the label to say

<source>Log composite form elements</source>

or something simlar

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, indeed, I see what you mean. Let me have a look, logging elements which cannot have values indeed doesn't make any sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, will use my branch for now because it fits my needs.
If you ask me, i would suggest no to indroduce the first config and just skip all composite fields. Only add the config to additionally skip specific form element types (e.g. staticText) because those don't have _isCompositeFormElement option set.
Have it configurable IMHO makes sense because people might introduce custom elemtents without values in their forms (this is why I need it).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree. Give me some time to properly play this through. Maybe you can push another PR in the meantime for the element type filter.

</trans-unit>
</body>
</file>
</xliff>
5 changes: 4 additions & 1 deletion Resources/Private/Language/de.Database.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
<source>Log form data</source>
<target>Formulardaten loggen</target>
</trans-unit>

<trans-unit id="formEditor.elements.Form.editor.finishers.LogFormData.includeHiddenElements.label">
<source>Store composite form element data?</source>
<target>Formulardaten von Composite-Elementen speichern?</target>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<target>Formulardaten von Composite-Elementen speichern?</target>
<target>Versteckte Elemente loggen</target>

</trans-unit>
</body>
</file>
</xliff>