Interesting approach for sure - but I personally would not do this by an abstract class and an interface, and generally it is considered bad practice to read values from ViewHelper instances this way using a public method. ViewHelpers should follow the "tell, don't ask" principle.

I suggest instead:

• getSlideRecords of your AbstractSlideViewHelper refactored to PageSelectService.
• getSlideRecords method adjusted to accept the slide etc. parameters as arguments instead of looking up in local $this->arguments.
• Implement usage of PageSelectService in ViewHelpers which should support sliding.
• Do not pass ViewHelper instance but rather pass the arguments needed to perform the fetching.
• Don't create new interface and abstract class; but
• Do add these additional arguments on all ViewHelpers where it makes sense.

Perhaps not clean but would ensure consistency:

• Add constants on PageSelectService which contain the expected names of the three slide related arguments, then use those constants as argument name. Putting these constants on the PageSelectService isn't exactly a nice solution, but the fact that this class is available is just about the only (VHS-context) thing that connects all these ViewHelpers.

And I suggest the following commit order:

1. PageSelectService method and constants, as one commit.
2. Refactoring current slide capable ViewHelpers to use this new method, as one commit.
3. Refactoring currently incapable ViewHelpers to add usage of service and registration of arguments, as one commit per ViewHelper. Right now I think there's only that one page-resources FAL ViewHelper to do.

And there's only white-space to nitpick over regarding CGL and formatting - everything that matters is spot on, both in code and commit messages, and the solutions (EDIT: speaking about the method bodies only - we can always shuffle the methods themselves around!) read like they should solve this in a nice way - but you should set your IDE to "remove whitespace from end of lines on save" and save these files to pass the cosmetic test ;)

OK, biggest reason for me to use the abstract class and interface was to avoid copying the argument declarations to every class and make it as generic as possible. It's a shame there is no kind of mixin system for this. There are not many ViewHelpers that need sliding, though, so it's not that big of a problem.

getSlideRecords of your AbstractSlideViewHelper refactored to PageSelectService.

The problem here is that the way of getting page-related resources is very different between tt_content and FAL. Should I simply insert an argument to select what kind of resources should be fetched? It also means that the code in Resource\Record\FalViewHelper would need to be duplicated or also moved to a utility class. Just moving it from there to PageSelectService is not possible as only the subclass Page\Resources\FalViewHelper can use sliding.

everything that matters is spot on, both in code and commit messages

Thanks, glad to see that the effort to blend in with the coding style used does not go completely unnoticed :)

but you should set your IDE to "remove whitespace from end of lines on save" and save these files to pass the cosmetic test ;)

Done. On a not completely unrelated note, the contributor guideline links are all dead on the new fluidtypo3.org page...

The problem here is that the way of getting page-related resources is very different between tt_content and FAL.

Yeah, I see your point. Then perhaps better to only refactor the reading of root line with sliding options to PageSelectService and leave the loop that reads the resources in each ViewHelper. I agree it would be preferable with a way to share this method, but even inside the loops we may need different behaviors for limit. Then later if we do get mixins we can refactor that code again - but for now, seems we're stuck with a bit of code duplication.

It's not that I'm deadly afraid of new abstract classes, but I've made the mistake of falling back on abstract classes too eagerly in FED, resulting in many ViewHelpers having to become TagBased ViewHelpers. All in all, code duplication and expecting to be able to use some sort of mixin at some point (if nothing else, then when Traits become possible in relation to required php version in TYPO3).

Regarding the dead links would you mind creating a new issue and just pasting the locations you found so far? Would help a lot :)

I agree it would be preferable with a way to share this method, but even inside the loops we may need different behaviors for limit.

What kind of different behaviours would be possible here? Even if say the limit should be applied per page and not the to the whole number of records, that would be easily implementable as a parameter to the PageSelectService function.

As a last suggestion, how about moving the code to PageSelectService, but keeping the PageRelatedResourcesInterface and implementing it either in the ViewHelpers itselves or extra utility classes? We avoid the abstract base class this way, but keep the slide utility function generic and duplication-free. Resoure\Record\FalViewHelper among others is already implementing RecordResourceViewHelperInterface after all, which is conceptually very similar. (Sorry to nitpick this far but yes, I really hate duplicate code.)

Sorry about the delay! I promise to have a re-evaluation for you as soon as I get some time.

What kind of different behaviours would be possible here? Even if say the limit should be applied per page and not the to the whole number of records, that would be easily implementable as a parameter to the PageSelectService function.

I'm thinking about things like localisation overlaying, workspace overlay and removal of deletion placeholders, applying additional attributes etc. Limit and how limit is applied is sufficiently generic that all implementers might use it so that's a candidate for a method argument, but all this other stuff likely involves operations that cannot/should not be universal.

As a last suggestion, how about moving the code to PageSelectService, but keeping the PageRelatedResourcesInterface and implementing it either in the ViewHelpers itselves or extra utility classes?

The whole problem with the interface isn't that it gets implemented, but that the classes which implement it are then expected perform tasks which really aren't ViewHelper tasks (or, at the very least, should reside in a utility or base class). And there's the catch 22 again - yes, your solution avoids the code duplication that my suggested solution inevitably will lead to, but it also forces ViewHelpers to act as something they're not designed to be - data providers for outside code.

Resoure\Record\FalViewHelper among others is already implementing RecordResourceViewHelperInterface after all, which is conceptually very similar. (Sorry to nitpick this far but yes, I really hate duplicate code.)

That's only part true ;) although yes, this is an interface and therefore yes, the methods are declared as public, you'll notice that we only use those methods as internal methods and have the interface to ensure they respect our desired implementation. It's a vital difference.

The clean solution in terms of respecting the framework, is the one that's not clean in terms of code duplication - but it is the one we need here. There is one other solution I consider but I tend to judge that as engineering overkill... engage braining:

Data Source wrappers

Basically this idea revolves around creating a new type of classes in VHS: DataSources. A DataSource might for example be able to return FAL resource objects by accepting a few key parameters for a query. Another DataSource might return content records, also using a method to accept query parameters. A DataSource could then extend other datasources - for example, a WorkspaceContentDataSource might extend the ContentDataSource and as post-processing, perform workspace overlaying of results.

That in itself is pretty basic but doesn't avoid code duplication. In order to ensure that we have a minimum of code duplication and a strict API, the following could be done:

• DataSource implements an interface which adds a getArgumentDefinitions method which returns an array of our own ArgumentDefinintion subclass instances for Fluid ViewHelpers.
• Our ArgumentDefinition subclass contains a method to return array($name, $type, $label, $required, $default) but is otherwise a standard ArgumentDefinition.
• ViewHelper creates instance of DataSource in method initializeArguments and calls getArgumentDefinitions on it to get the array of additional arguments, then adds each of those, in a loop, to registered arguments using for example call_user_func_array(array($this, 'registerArgument'), $argument->getRegistrationParameters()).
• Then, when reading the data, each ViewHelper creates an instance of the desired DataSource and passes $this->arguments to a fetch or similar method also declared in the interface.

The reason for this structure of ViewHelper:

• Avoids code duplication and ensures argument consistency as well as all arguments being supported on ViewHelpers which utilise the same DataSource type.
• Has to be a slightly complex solution using initializeArguments to make a DataSource instance this way, because of the combination of visibility of argument definition storage on ViewHelper base class being private and the registerArgument method not being public; it's neither possible to simply append ArgumentDefinitions, nor is it possible to let the DataSource append the additional arguments.
• Nicely testable without actual data sources being present.

However, one could quite easily argue that this is an over engineered pattern to solve a fairly simple undesirable aspect: code duplication. It is, however, a much better separation of concerns, it is friendlier to test and would make it easier for users to create their own types of new FAL-based ViewHelpers for example. It means we can drop some of the interfaces and abstract classes currently being used to solve this type of problem and last but not least, it may give us the tool we need to make our page-related ViewHelpers more maintainable (because trust me, currently they're really complex to maintain regarding arguments as well as data sourcing).

Your thoughts on this completely-in-the-opposite-direction idea? If you'd be up to implementing something like that I can offer my help on a bit more real-time basis via IRC ;)

Sounds good to me - it's basically what I intended to do, but done right. I'll certainly try to implement it, but I don't know yet when I'll have enough time. I'll contact you in IRC when the time comes and make a new, clean pull request.

I started implementing your suggestion, and would like to have further input on the direction that should be taken. I have commited my work-in-progress code at 2e26001.
I am unsure where to make the split between the responsibilities of the DataSource and the ViewHelper. For example, the FAL DataSource needs a page UID to fetch records from. Should the default value (the UID of the current page) be provided by the ViewHelper or determined in the DataSource automatically?

As far as sliding goes, I moved the page UID collection to PageSelectService. The sliding itself can easily be customized by overriding getSlideRecordsFromPages in AbstractPageSlideDataSource if a different limit etc. handling is needed. The default implementation should be fine for FAL and content for the time being.

This is an automated comment based on an automated formal commit review.

Your pull request contains formal errors. Comments have been assigned to each commit in your pull request - please review and adjust. Feel free to ask for help if you need it!

Once again, sorry about the delay @pkerling - I'm a little hesitant about this one because of the amount of code it affects. I've looked through your code on each iteration but it still feels to me like it complicates the class hierarchy too much, but for the most part the methods are great. In other words, the only thing that worries me is the implementation and the consequences it has. I also have to retract a bit on my suggestion of DataSources - although that solution would allow us to do this, those models really belong in the core itself (as a part of FAL).

Recently I began considering the use of Traits to add specific capabilities to a ViewHelper, for example a RenderAssistTrait which contains a $this->renderChildrenWithVariables(array $variables) (á la current ViewHelperUtility) and $this->registerAsArgument method. We could use such a Trait or set of Traits that contain $this->registerSlideArguments() methods (á la registerUniversalTagAttributes) and getSlideRecords. Such a Trait could then be changed in a subclassed ViewHelper to use another resource type for the sliding, but would re-use the same registerSlideArguments method. Maybe that can even be done by just implementing two (or more) Traits.

This could also be further extended to place FAL capabilities in a Trait that any ViewHelper can implement and call a simple register*****Arguments method from initializeArguments and gain access to methods like getResources inside the ViewHelper.

Traits would be the ideal way to avoid code duplication without affecting the actual class hierarchy (we can even loosen it up a bit more which would be great also for transparency), but it has a heap of angles that should be considered. I think after all this work you have a great perspective on this so I would like to invite you to discuss this on IRC so we can get into all the little details and maybe create a working proof of concept together. You can use the support chat plugin if you don't use an IRC client. The best time to catch me is from 19:00 to around midnight, or on the weekend - and you're welcome to message me directly.

Almost forgot to mention: it looks like most of the method bodies you already wrote could be used for the suggested Traits, so hopefully I haven't wasted your time with this re-planning. I very much appreciate the time you spent on this!

Hmm, I'm also concerned about side effects this may introduce. Can you please rebase it, so we can test? While you're at it, please correct the codestyle errors; thank you!

@cedricziel: It has become clear that it will not be implemented/merged as it is now, so a rebase does not make sense. I will talk with NamelessCoder about the direction of this feature and implement a different solution then, but it might take some time. What side effects are you concerned about?

I've scrambled together a rough draft at a9be9e6 to use as a starting point for discussion - sorry that it's taken so long. It's nothing exiciting though, only AbstractSlideViewHelper from the first draft implemented as SlideViewHelperTrait. I'll try to contact you in IRC then about the FAL stuff.