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

[FEATURE] SplitViewHelper #856

Merged
merged 1 commit into from
Mar 15, 2024
Merged

[FEATURE] SplitViewHelper #856

merged 1 commit into from
Mar 15, 2024

Conversation

s2b
Copy link
Contributor

@s2b s2b commented Jan 14, 2024

This patch adds a new <f:format.split> ViewHelper to Fluid Standalone. It is named "split" instead of "explode" to be consistent with the naming of JavaScript.

The ViewHelper aims to support everything that TYPO3's utility functions "GeneralUtility::intExplode()" and
"GeneralUtility::trimExplode()" provide.

  • items in the resulting array can be trimmed of whitespace
  • items in the resulting array can be converted to integers
  • empty items in the resulting array can be removed
  • the "limit" option behaves similar to PHP's explode() behavior

In a following patch, its counterpart "join" will be added.

@mbrodala
Copy link
Contributor

TBH I see multiple viewhelpers / syntax features here:

  1. Splitting a string with a custom delimiter
  2. Filtering "empty" items from an array
  3. Converting array items to integers

IMO this should be split accordingly to have small composable parts which could be chained:

{array -> f:iterator.split(delimiter: ",") -> f:iterator.filterEmpty() -> f:iterator.convert(type: "int")}

Notice that "iterator" would be a better NS here to hint at the subject type these viewhelpers operate on. The actually do not perform formatting (convert to string) as known in Fluid so far.

@garvinhicking
Copy link
Contributor

My two cents:

(Preface: Generally I'm not an advocate of using too much logic in fluid templates. The complexity/barrier should be low, so that integrators without too much coding knowledge can find their ways.)

I can see a need for converting a string to an array (for iteration) or the other way round (joining an array) so that this data can easily be passed to data-attributes or javascript-calls, where integrators have no ability/entrypoint to do that within a Controller or Service.

While the chaining is cool for developers, I'm afraid it adds too much cognitive complexity to a task, where integrators are more accustomed to specifying parameters (like "nonEmpty=true" and whathaveyou). A chained syntax looks a bit like map/reduce, which developers may find easy to read, but can create panic for "normal people"?

I think to stay within the "format" namespace would better suit established knowledge, where people search for any string transformations in that scope? TYPO3-fluid uses "transform" which would also fit somehow, but it's current application is focussed at HTML and not array/string conversion.
The NS "iterator" would lead me towards "f:for" and "f:cycle" or so, when in reality it's not addressing iteration, but rather type conversion/modification.

Closing note: Please do use the keywords "implode" and "explode" in the ViewHelper description, so that PHP developers can easily find the viewhelper (I myself wouldn't search for "split").

@mbrodala
Copy link
Contributor

Indeed, the input is not an iterator, so the NS should actually be text or string, so f:text.split or similar. Again, it's not a formatting as one is used to so IMO format is not the correct NS.

@garvinhicking
Copy link
Contributor

How about:

  • Transform::split/join
  • Convert::...
  • Data::...
  • Operations:..

IMO "formatting" would still fit, because I "format a string as array", and would use a well-known namespace.

(Filed under: There are only two hard things in Computer Science: cache invalidation, naming things and off-by-one errors)

@benjaminkott
Copy link
Member

Please let us avoid mistakes of the past and not add more bloat to our templates for base-level functionality.
It should just be <f:split /> no need to add more unnecessary stuff here.

@s2b
Copy link
Contributor Author

s2b commented Mar 12, 2024

Can you be more specific? What is unnecessary? What should and shouldn't the VH support?

@benjaminkott
Copy link
Member

@s2b it´s about the placement debate not about the functionality.

<f:format.trim> -> <f:trim>
<f:text.split> -> <f:split>

@mbrodala
Copy link
Contributor

Maybe we should also take some inspiration from Twig here: https://twig.symfony.com/doc/3.x/filters/index.html

The similarly have format_* filters and toplevel filters ...

@benjaminkott
Copy link
Member

Maybe we should also take some inspiration from Twig here: https://twig.symfony.com/doc/3.x/filters/index.html

The similarly have format_* filters and toplevel filters ...

we almost need all of those, added the join on https://review.typo3.org/c/Packages/TYPO3.CMS/+/83442
split was next on my list, then merge

Copy link
Member

@benjaminkott benjaminkott left a comment

Choose a reason for hiding this comment

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

If we continue this style, every view-helper is responsible for everything. We should stay to the basics. See twig or JS prototype implementation.

value -> split(separator, limit)

src/ViewHelpers/Format/SplitViewHelper.php Outdated Show resolved Hide resolved
src/ViewHelpers/Format/SplitViewHelper.php Outdated Show resolved Hide resolved
src/ViewHelpers/Format/SplitViewHelper.php Outdated Show resolved Hide resolved
@garvinhicking
Copy link
Contributor

Please let us avoid mistakes of the past and not add more bloat to our templates for base-level
functionality.
It should just be <f:split /> no need to add more unnecessary stuff here.

Our current "manipulation" viewhelpers all use the "format" hierarchy, and in the main hierarchy we only have quite specific structural/operational helpers:

  • variable
  • if/then/else
  • spaceless
  • section
  • layout
  • inline
  • for/groupedFor/cycle
  • debug
  • comment
  • count
  • case/defaultCase
  • alias

When we add "data/string/array manipulation" to that main level, it may become odd for integrators why one is in "format" and the other is not...?

I agree, adding a new top level hierarchy feels like "too much". In that case I'd opt for using the "format" namespace though, to keep documentation and expectations in line.

@s2b
Copy link
Contributor Author

s2b commented Mar 13, 2024

Thank you all for this helpful feedback. Now that I have a better overview of the different opinions about this, I can get started with other ViewHelpers as well.

Summary:

  • A ViewHelper should be simple and should only have a small feature set.
  • Naming should be consistent with either PHP or JS, but not necessarily TYPO3 Core.
  • The folder structure (especially format vs. the root folder) is inconsistent now.
  • We probably need some kind of iteration functionality (like map @andreaskienast mentioned above).
  • More ViewHelpers are missing.

For this ViewHelper, I will adjust the following:

  • Remove unnecessary functionality.
  • Rename parameters and (probably) move to root folder.

I will think a bit more about the folder structure discussion as well as the consequences of changing it (like VH documentation, backwards compatibility, ...).

@s2b
Copy link
Contributor Author

s2b commented Mar 14, 2024

I've uploaded a new version. I moved the VH to the root folder and removed all arguments except for limit, which is consistent with explode.

The SplitViewHelper splits a string by the specified separator, which
results in an array. The number of values in the resulting array can
be limited with the limit parameter, which results in an array where
the last item contains the remaining unsplit string.

This ViewHelper mimicks PHP's :php:`explode()` function.

Split with a separator:

```xml
<f:split value="1,5,8" separator="," /> <!-- Output: {0: '1', 1: '5', 2: '8'} -->
```

Split using tag content as value:

```xml
<f:split separator="-">1-5-8</f:split> <!-- Output: {0: '1', 1: '5', 2: '8'} -->
```

Split with a limit:

```xml
<f:split value="1,5,8" separator="," limit="2" /> <!-- Output: {0: '1', 1: '5,8'} -->
```
@s2b s2b merged commit b30e29c into main Mar 15, 2024
6 checks passed
@s2b s2b deleted the feature/splitViewHelper branch March 15, 2024 09:58
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.

6 participants