Skip to content

Added render class for Viewfield.#4

Closed
marcus-n3rd wants to merge 7 commits into
wellebee:8.x-2.xfrom
marcus-n3rd:render-class
Closed

Added render class for Viewfield.#4
marcus-n3rd wants to merge 7 commits into
wellebee:8.x-2.xfrom
marcus-n3rd:render-class

Conversation

@marcus-n3rd

Copy link
Copy Markdown

Absorbed the following functions into the class:

  • viewfield_element_info()
  • viewfield_pre_render()
  • viewfield_post_render()
  • _viewfield_get_view_args()
    -- This was made getViewArgs() which is a protected method

Left rendering for the theme, i.e.: removed $output = render ($view_el);, other than that I didn't touch any logic within the functions.

Thank Metal Toad for paying my hours for this. Lightly tested this on a client's project and I'd say it's now eligible to go from sandbox to full project status.

Lukas von Blarer and others added 5 commits June 9, 2016 03:55
Absorbed the following functions into the class:
- viewfield_element_info()
- viewfield_pre_render()
- viewfield_post_render()
- _viewfield_get_view_args()
-- This was made getViewArgs() which is a protected method

Left rendering for the theme.
* adding-cache-tags:
  Using buildRenderable() to add cache metadata
  Remove kint()
  Adding cache tags

# Conflicts:
#	viewfield.module
This is so that the arguments have had an opportunity to get aggregated by the Viewfield class.
@marcus-n3rd

Copy link
Copy Markdown
Author

I combined the changes from:

@derhasi derhasi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@marcus-n3rd, thanks for your work. I'm just starting getting into the issues again after some months.

Here are some issues I found from a "meta" perspective. I will have a closer look at the functionality itself later.

* Contains \Drupal\Core\Render\Element\Viewfield.
*/

namespace Drupal\Core\Render\Element;

@derhasi derhasi Sep 30, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It should not use core's namespace, but Drupal\viewfield\... instead.

*/
class Viewfield extends RenderElement {

public function getInfo() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This needs at least function documentation. I guess {@inheritdoc} should do its job here.

);
}

public static function preRenderViewfield($element) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This needs some proper function documentation and type declaration.

return $element;
}

public static function postRenderViewfield($content, $element) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This needs some proper function documentation and type declaration too.

/**
* Perform argument replacement
*/
protected function getViewArgs($vargs, $entity_type, $entity) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There needs to be some parameter documentation.

@marcus-n3rd

Copy link
Copy Markdown
Author

Oh fudge. While step debugging through in order to double check the argument types, I found that none of Viewfield class is actually used. 🤦 I'm going to run through and see if there are parts that can be consumed and used in the other classes.

@marcus-n3rd

Copy link
Copy Markdown
Author

I am really pondering the value of having it as a RenderElement. Could the functionality just be moved into the ViewfieldDefaultFormatter (with the theme name changing to simply viewfield rather than viewfield_formatter_default)? Is the viewfield_stack static variable still needed with Drupal 8's render caching, because that would get rid of the need of the pre/post rendering functions/methods?

@luksak

luksak commented Oct 14, 2016

Copy link
Copy Markdown

Well, I was trying to fix the caching issues and added that in the field formatter. As Berdir suggested, we should move that somewhere else where the render array is being built and that is what you approach does, right?

@marcus-n3rd marcus-n3rd mentioned this pull request Oct 24, 2016
@marcus-n3rd

Copy link
Copy Markdown
Author

Closing in preference of #8.

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.

4 participants