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.

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

Conversation

johfeu
Copy link

@johfeu johfeu commented Sep 4, 2024

intitially based on the PR #33 but taking the review comments into account. It is tested against the latest version of the extension and TYPO3 v12.4

This pull request introduces a feature to control the saving of values from hidden or composite form elements. In current behavior, all form elements are saved by default, even if they do not hold any value, leading to empty columns in log records and exports. This is different to how the core EmailFinisher handles it.

  1. includeHiddenElements Finisher Option:

    • A new finisher option, includeHiddenElements, has been introduced.
    • When set to false, this option prevents values from hidden or composite elements from being saved.
    • This behavior is similar but not the same to the existing EmailFinisher, which only includes fields that actually hold a value.
    • can be toggled in the inspector
  2. skipElementsTypes Finisher Option:

    • The new option skipElementsTypes allows users to specify which form elements should be excluded from being saved.
    • This is configured as a comma-separated list of element types.
    • By default, this option includes ContentElement and StaticText elements, as these elements do not hold values.

Impact:

These changes will help reduce unnecessary data storage and improve the efficiency of data handling in log records and exports. By allowing customization of which elements are saved, users can better manage their form data and avoid clutter from empty values.

Copy link
Member

@mbrodala mbrodala left a comment

Choose a reason for hiding this comment

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

Great suggestions, thanks for these! Can you split this into two separate PR? Also make sure to add some unit tests to verify these new options.

@@ -32,6 +33,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?

Comment on lines +112 to +115
// 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()
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.


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.

@@ -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

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 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>

@mbrodala mbrodala changed the title [FEATURE] Add option to skip saving composite fields Add option to skip saving composite fields Sep 11, 2024
@mbrodala mbrodala marked this pull request as draft September 11, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants