[RTM] Added PaletteManipulator

[aschempp]

PaletteManipulator is finally a helper tool to add fields in existing DCA palettes. It handles the following cases:

• Add field after existing field
• Add field before existing field
• Append field to legend
• Prepend field to legend
• Add new legend and fields after existing legend
• Add new legend and fields before existing legend

It is intentionally kept as simple as possible. I did not want to introduce object-oriented palettes, as I doubt that would be backwards compatible. The class is simply a helper to string replace.

I'm actually in need of such a tool to continue working on #471. There's a field tl_layout.calendarfeeds and tl_layout.newsfeeds which should not be in core-bundle (see https://github.com/contao/core-bundle/blob/master/src/Resources/contao/dca/tl_layout.php#L100).

Example how to add tl_layout.calendarfeeds fields:

// CalendarBundle/Resources/contao/dca/tl_content.php
$GLOBALS['TL_DCA']['tl_layout']['palettes']['default'] = 
    PaletteManipulator::append('feed_legend', 'calendarfeeds')
    ->setFallback(
        PaletteManipulator::afterLegend('picturefill_legend', 'feed_legend', 'calendarfeeds')
    )
    ->applyTo($GLOBALS['TL_DCA']['tl_layout']['palettes']['default'])

What it does:

1. Try to append field calendarfeeds (could also be an array of fields) to legend feed_legend.
2. If 1. fails, add new legend feed_legend with new fields after picturefill_legend
3. If 2. would fail (which it should not in core…), it would simply append the new legend at the end

[Toflar]

Cool stuff! I do think, you should not use static methods though.

$pm = new PaletteManipulator();
$pm->append('feed_legend', 'calendarfeeds');

works just as well as the static methods. But compared to the static API it e.g. allows us to think about adding a service for it (a factory service) so we can inject it into other services. When I think about the fact that we want to have YAML for dca files one day, I'll need a service that handles my palette manipulation.

Also see symfony/symfony#15850 why Symfony has changed the CSS Selector API.

[aschempp]

I disagree.

1. This class can't be a service because it has state. The static methods are basically the factory for this class, you don't have to use them if you don't want to.
2. The code example would not work, because you could call ->append() or other methods multiple times. The class can't handle multiple appends and prepends. And overwriting the state would be very strange.

No idea how to solve YAML, but I don't think that should be related. This is just a helper class for our current "broken" palette problem, I would probably not use a string for palettes in YAML :D

[leofeyer]

I like it.

[discordier]

While I like the overall approach for making things more easy, I feel like it is guiding people in the wrong direction.

I mean, the long term goal is to have DCA file contain simple "information" about the table structure and edit masks. This "simple information" should be able to be contained in YAML and all other things.
The approach in this PR will guide people to, yet again, push more logic into their dca files which should not contain any logic or classes or anything else aside from defining the dca array.

Therefore I second the statement by @Toflar above, this has to be transformed into an service and/or preprocessor.
@tristanlins and I wrote something quite similar back in time for MetaPalettes, which provided alternative syntaxes as array notation (i.e. "foobar before picture" would place field foobar right before the picture field).

Aside from these concerns, I think it should have a more fluent interface, like the config builder.
I also dislike the current approach of defining the "fallback" as separate object instance.
I think something like this would be more intuitive:

// CalendarBundle/Resources/contao/dca/tl_content.php
$GLOBALS['TL_DCA']['tl_layout']['palettes']['default'] = 
  // Create a new manipulator
  PaletteManipulator::create()
    // we want to manipulate the feed_legend
    ->manipulateLegend('feed_legend')
      // if that one is not found, we want special handling.
      ->ifLegendNotFound()
        // we want to create it.
        ->createLegend('feed_legend')
          // we want it after the picturefill_legend.
          ->after('picturefill_legend')
          // the inital state shall be hidden ("xyz_legend:hide")
          ->hidden()
        // end of not found handling
        ->end()
      // end of legend not found
      ->end()
      // append the field to the legend.
      ->appendField('calendarfeeds')
    // end legend handling
    ->end()
    // finally apply everything to the passed palette
    ->applyTo($GLOBALS['TL_DCA']['tl_layout']['palettes']['default'])

Using such an approach, seems more intuitive. It could even eventually be defined in any compiler pass which processes the DCAs before caching (long term goal).

[leofeyer]

As briefly discussed on Mumble, this is what we want:

PaletteManipulator::create()
    ->addLegend('legend', 'afterThisLegend') // or at the end
    ->addLegend('legend', ['afterThisLegend', 'orAfterThisLegend']) // or at the end
    ->addField('field', 'afterField', 'orAtTheEndOfThisLegend') // add the legend if it does not exist
    ->addField('field', ['afterThisField', 'orThisField'], 'orAtTheEndOfThisLegend') // add the legend if it does not exist
    ->addField(['thisField', 'andThisField'], 'afterField', 'orAtTheEndOfThisLegend') // add the legend if it does not exist
    ->addField(['thisField', 'andThisField'], ['afterThisField', 'orThisField'], 'orAtTheEndOfThisLegend') // add the legend if it does not exist
    ->applyToPalette('thisPalette', 'table')
    ->applyToPalette('thatPalette', 'table')
    ->applyToSubpalette('thisSubpalette', 'table')
;

[Toflar]

Just a small reminder: Pls consider manipulating subpalettes too. So maybe applyToPalette() and applyToSubpalette() or so. You choose :)

[leofeyer]

I also changed append() to addField(), because there is also an addLegend() method.

[aschempp]

I've rewritten everything based on our feedback. Now 🍻

[bytehead]

happy 🍻 ! :)

[Toflar]

Looks waaaay better!!! :) I'm happy now. Almost :D
According to sebastianbergmann/phpunit#1914 we should not use the static version of assertX() calls and am I blind or are the tests for the fallbacks missing?

[aschempp]

Yeah, and someone should write 1000 unit tests for all possible cases :D

[aschempp]

I've changed everything to $this->assert… and added a lot more unit tests for 100% code coverage 😎

[leofeyer]

Merged in d15585c.

[aschempp]

FYI I'm not very happing with your merge. You removed a lot of my comments on how methods and arguments work, and I don't think thats a good thing…

[leofeyer]

AFAIR, I only shortened the comments and did not remove them.

[leofeyer]

Also, I had to fix several CS and code duplication issues. Actually, I still am on it right now.

[aschempp]

Yeah, you shortened a lot: 5155f16#diff-dcb2151bb12749e152bdd80279ed8d0fL67
Dunno why you would shorten a comment and remove useful information…

[aschempp]

Btw. we recently also discussed not to add useless names for variables like in 5155f16#diff-dcb2151bb12749e152bdd80279ed8d0fR334

[leofeyer]

Yes, we have discussed it but we have not agreed on anything. If we agree on skipping the description, we would need to remove it everywhere and not just in a single class (consistency!).

[leofeyer]

I was able to refactor the methods so there is no more code duplication and there are no more C rated methods. However, the class itself is still C rated (the only C rated class in the new code), because it is much too complex. And I don't really like that :)

[aschempp]

If we agree on skipping the description, we would need to remove it everywhere and not just in a single class (consistency!).

I'm not gonna discuss that, that's just silly…

[discordier]

Well, of course we should keep comments as descriptive as possible. Where's the point of comments when they do not make sense? This is btw. the only downside I see with Symfony, their class comments are next to useless.

In order to drop the class rating from C to A, I'm afraid, we will need to refactor it to a bunch builder classes, which you have already decided against.

[leofeyer]

In order to drop the class rating from C to A, I'm afraid, we will need to refactor it to a bunch builder classes, which you have already decided against.

Extracting the implode() and explode() methods would already improve the rating to B. And these two methods could be reused in the data container classes, which also need to split palette strings.

A reasonable thing to do IMHO, but @aschempp is not willing to discuss today.