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

Allow record sliding in page.resources.fal #679

Closed
wants to merge 6 commits into
base: development
from

Conversation

Projects
None yet
3 participants
@pkerling
Copy link
Contributor

pkerling commented Sep 30, 2014

Page\Resources\FalViewHelper was until now missing the feature to use record sliding for page-related media. That is however commonly required for dynamic page background images and such and I wanted to avoid falling back to TypoScript.

It made quite a challenge to separate the already existing sliding code from AbstractContentViewHelper into a separate class and I'm not sure whether the result is quite right. If you have suggestions concerning the implementation of this feature, I'd be happy to rework it.

pkerling added some commits Sep 30, 2014

[FEATURE] Introduce AbstractSlideViewHelper
Record sliding was traditionally handled solely by content view helpers. It is however desirable to extend sliding support to a greater range of view helpers, most prominently Page\Resources\FalViewHelper.

AbstractSlideViewHelper implements all functionality commonly required for sliding and should thus be used as base class for all view helpers that, optionally, want to support sliding, even if only in a subclass.

Compared to the original implementation in Content\RenderViewHelper, a few changes were introduced:
* $limit is now referring to the total number of records, not the number of records rendered per page. This is important when using $slideCollect.
* $slideCollectReverse now selects the correct page range from the rootline when setting it to a value greater than zero.
* $slideCollectReverse can now be set to -1 and used without also setting $slide to -1.
[FEATURE] Make AbstractRecordResourceViewHelper inherit from Abstract…
…SlideViewHelper

This is required for using record sliding in Page\Resources\FalViewHelper which is a subclass of AbstractRecordResourceViewHelper.
[FEATURE] Make Page\Resources\FalViewHelper support record sliding
This is achieved by utilizing the new functionality introduced by AbstractSlideViewHelper. Custom ordering of the records is not supported, but probably also not needed.
[TASK] Make AbstractContentViewHelper inherit from AbstractSlideViewH…
…elper

The sliding-related code from AbstractContentViewHelper was adopted into AbstractSlideViewHelper and thus does not need to be duplicated here.
[FEATURE] Adopt Content\Random\GetViewHelper to correctly support rec…
…ord sliding

Content sliding was already supported, but lacked some features. Now it is possible to use $slideCollect and render more than one randomly selected content element by increasing $limit.
@NamelessCoder

This comment has been minimized.

Copy link
Member

NamelessCoder commented Sep 30, 2014

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

@pkerling

This comment has been minimized.

Copy link
Contributor

pkerling commented Oct 1, 2014

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

@NamelessCoder

This comment has been minimized.

Copy link
Member

NamelessCoder commented Oct 1, 2014

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

@pkerling

This comment has been minimized.

Copy link
Contributor

pkerling commented Oct 2, 2014

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

@NamelessCoder

This comment has been minimized.

Copy link
Member

NamelessCoder commented Oct 6, 2014

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

@NamelessCoder

This comment has been minimized.

Copy link
Member

NamelessCoder commented Oct 23, 2014

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

@pkerling

This comment has been minimized.

Copy link
Contributor

pkerling commented Oct 28, 2014

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.

@pkerling pkerling closed this Oct 28, 2014

@pkerling

This comment has been minimized.

Copy link
Contributor

pkerling commented Dec 13, 2014

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.

@pkerling pkerling reopened this Dec 13, 2014

$this->registerArgument('slideCollect', 'integer', 'Amount of levels which shall get walked up the rootline and have their records collected. For infinite sliding (till the rootpage) set to -1. If set, this value overrides $slide', FALSE, 0);
$this->registerArgument('slideCollectReverse', 'boolean', 'Normally when collecting records the elements from the actual page get shown on the top and those from the parent pages below those. You can invert this behaviour (actual page elements at bottom) by setting this flag))', FALSE, 0);
}

This comment has been minimized.

@NamelessCoder

NamelessCoder Dec 13, 2014

Member

Whitespace found at end of line

if (NULL === $limit && FALSE === empty($this->arguments['limit'])) {
$limit = (integer) $this->arguments['limit'];
}

This comment has been minimized.

@NamelessCoder

NamelessCoder Dec 13, 2014

Member

Whitespace found at end of line

}
}
} while (0 < count($storagePageUids));

This comment has been minimized.

@NamelessCoder

NamelessCoder Dec 13, 2014

Member

Whitespace found at end of line

$as = $this->arguments['as'];
if (TRUE === empty($as)) {
return $resources;
}

This comment has been minimized.

@NamelessCoder

NamelessCoder Dec 13, 2014

Member

Whitespace found at end of line

@NamelessCoder

This comment has been minimized.

Copy link
Member

NamelessCoder commented Dec 13, 2014

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!

if (TRUE === (boolean) $this->arguments['render']) {
$contentRecords = $this->getRenderedRecords($contentRecords);
}
}
return $contentRecords;
}

This comment has been minimized.

@NamelessCoder

NamelessCoder Dec 13, 2014

Member

Whitespace found at end of line

@NamelessCoder

This comment has been minimized.

Copy link
Member

NamelessCoder commented Feb 8, 2015

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.

@NamelessCoder

This comment has been minimized.

Copy link
Member

NamelessCoder commented Feb 9, 2015

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!

@cedricziel

This comment has been minimized.

Copy link
Member

cedricziel commented Feb 19, 2015

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!

@pkerling

This comment has been minimized.

Copy link
Contributor

pkerling commented Feb 19, 2015

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

@pkerling

This comment has been minimized.

Copy link
Contributor

pkerling commented Apr 27, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment