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

Add note about using RendererInterface::render() #39

Open
wants to merge 2 commits into
base: 2.11.x
Choose a base branch
from

Conversation

aleksip
Copy link
Contributor

@aleksip aleksip commented May 1, 2020

Q A
Documentation yes
Bugfix no
BC Break no
New Feature no
RFC no
QA no

Description

Adding a note about the possibility to use RendererInterface::render() instead.

Signed-off-by: Aleksi Peebles <aleksi@iki.fi>
Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!

Only small changes are necessary so that the information can be included in the documentation.

docs/book/helpers/partial.md Outdated Show resolved Hide resolved
@froschdesign froschdesign added this to In progress in Documentation: improvements via automation May 1, 2020
Signed-off-by: Aleksi Peebles <aleksi@iki.fi>
Comment on lines +82 to +84
> argument. You might prefer to do this if your model is an array, or an
> object that implements `ArrayAccess`. See the documentation for
> [`PhpRenderer`](../php-renderer.md#render) for more information.
Copy link
Member

Choose a reason for hiding this comment

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

You might prefer to do this if your model is an array, or an object that implements ArrayAccess.

The result is the same with the Partial helper and render method for this.

But the Partial helper supports some more:

if (null !== ($objectKey = $this->getObjectKey())) {
$values = [$objectKey => $values];
} elseif (method_exists($values, 'toArray')) {
$values = $values->toArray();
} elseif (! $values instanceof Traversable) {
$values = get_object_vars($values);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally put the note in the very beginning as I considered it to be quite important for deciding whether to use Partial or not.

It seems that historically, when render() did not support a $values argument, Partial was much more useful, but what is left now is just this additional support for handling data. One might even consider the current introduction ("is used to render a specified template within its own variable scope") to be misleading?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm referring particularly to this commit, which seems to have changed the nature of Partial: e0825a0

Copy link
Member

Choose a reason for hiding this comment

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

I would not put so much emphasis on original or old behavior. The user should receive instructions on how to use the helper. That the render method is used is an important hint, but it is also important that the helper offers more. (See "What is a model?")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, currently the documentation seems to put too much emphasis on the old variable scope feature that has since been added to RendererInterface and is now only proxied by Partial.

I would even consider whether the model handling functionality that is left is valuable enough to keep Partial instead of deprecating it? The objectKey feature for example seems to be equivalent to a simple render($nameOrModel, [$key => $object]) call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into this more I see that Partial and its model are used by other parts of the framework, so I am guessing that there are good reasons for keeping the additional model support. And maybe in any case for keeping Partial as the recommended way to always render partial templates, since specific functionality can then be easily added later?

As a developer using the framework for the first time, interested in rendering partial templates in their own variable scope, I have however been confused about:

  • the difference between RendererInterface::render() and Partial, and
  • whether I should still use Partial if it only seems to add overhead in my specific use case, where I will not be needing the extended model support.

I am still unsure about the right answers to these questions, and I would love to find the answers in the documentation. This is what I was attempting to do with the PR. If there are additional changes that are needed I am happy to update the PR accordingly, but for the above reasons I would need some more guidance.

Copy link
Member

Choose a reason for hiding this comment

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

I dislike the entire page and the missing code samples for the different model types.

I will rewrite the page:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fantastic, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

No, thank you for bringing up this topic.


Btw. please have a look at the projects, there will you find "Documentation: cookbook recipes".
More ideas are needed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will have a look!

@froschdesign froschdesign self-assigned this May 5, 2020
@geerteltink geerteltink changed the base branch from master to 2.11.x September 11, 2020 12:05
@geerteltink geerteltink added this to the 2.11.5 milestone Sep 11, 2020
@boesing boesing removed this from the 2.11.5 milestone Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants