Update the render method in the Twig class

[solventt]

I remind that the Slim allows to define a custom route invocation strategy that determines arguments coming into the callable request handler (see Slim's docs).

And current implementation of the render() method assumes that the handler's arugments will necessarily contain the ResponseInterface object. But I, for example, use my own rout strategy that allows to omit unnecessary arguments, including the ResponseInterface object.

So I offer to make the render() method flexible - so that the user can choose the most convenient form of the method:

$view->render($response, 'example.html', ['name' => 'Josh'])

// OR

$view->render('example.html', ['name' => 'Josh'])

i.e. the $response may or may not be among the arguments of the method.

What was done:

1) Moving psr/http-factory from the require-dev section to the require (for having ResponseFactoryInterface in the Twig class) in the composer.json

2) Updating the render method (including the doc block)

3) Adding the $responseFactory protected property and setResponseFactory method (we can't inject a response factory to/in the constructor because ResponseFactoryInterface assumes different PSR-17 implementatons used by users)

4) The testRender() method was split into two methods: testRenderWithResponseFactory() and testRenderWithoutResponseFactory(). The code that initializes a $view, $mockBody, $mockResponse, $mockResponseFactory for the two test methods was put in a separate auxiliary method.

5) Adding the testInvalidArgumentInRender() and testNotDefinedResponseFactoryInRender() test methods.

6) Removing 2 unused class imports in 'use' section (Twig\Loader\FilesystemLoader and Slim\Views\TwigContext) in TwigTest

7) Replacing deprecated setMethods() with onlyMethods() (sebastianbergmann/phpunit#3687 and sebastianbergmann/phpunit#3769) in testAddRuntimeLoader()

8) Updating README.md

[t0mmy742]

While I find it can be useful (your case is a good example), I personally do not like the fact that render method can do two "different" things based on its parameters. It means loosing a part of typed parameters and doing more verifications for each calls.

Maybe we could keep original method and just create a new one like this (method name is not really good) :

public function renderFromResponseFactory(string $template, array $data = []): ResponseInterface
{
    if (!isset($this->responseFactory)) {
        throw new RuntimeException('Response factory is not defined');
    }

    return $this->render($this->responseFactory->createResponse(), $template, $data);
}

[solventt]

@t0mmy742 your method is even longer than the original method . Compare:

$view->render($response, 'example.html', ['name' => 'Josh'])         // original
$view->renderFromResponseFactory('example.html', ['name' => 'Josh']) // your method

Where is the optimization?

2) Quote: "It means loosing a part of typed parameters and doing more verifications for each calls..."

Where's the problem here? Lack of typed parameters does not make a method unreliable. Are you afraid to write a few extra IF conditions? OK, then look at the run() method of PhpUnit - It's very huge with 19 IF conditions! Does this mean that we have to abandon PhpUnit? Look at the Symfony internal code - you will find many methods with IF conditions - for examle, the __construct method of the CsrfTokenManager class. Does this mean that people should give up using Symphony?

[l0gicgate]

We are not going to modify the original method. In software development, when you make one thing try and do two things you introduce complexity in multiple areas.

One of the SOLID principles is the Open/Closed principle, open for extension, closed for modification. During a major release cycle, I typically like to follow this advice.

I'm open to extending with a solution like @t0mmy742 proposed but closed to modifying the existing implementation.

[solventt]

@l0gicgate If you follow SOLID principles then you are certainly right. And it is your package - and it is up to you to decide what to do. But let me ask a philosophical question that I ponder from time to time: if the initial release didn't provide for flexibility in method operation, does it mean that we should be hostage to SOLID? For example, a person forgets to include some useful thing in a method, and after the release thinks, "Yes I forgot, but SOLID doesn't allow changes."

But then if one follows SOLID principles, it turns out that software changes that break backward compatibility also contradict the "Open/Closed principle". For example, when you released version 4 of Slim, you rewrote some components (https://www.slimframework.com/docs/v4/start/upgrade.html) - this is not a modification but a change, hence it violates the "Open/Closed principle". I have heard different opinions on this.

[l0gicgate]

But then if one follows SOLID principles, it turns out that software changes that break backward compatibility also contradict the "Open/Closed principle". For example, when you released version 4 of Slim, you rewrote some components (https://www.slimframework.com/docs/v4/start/upgrade.html) - this is not a modification but a change, hence it violates the "Open/Closed principle". I have heard different opinions on this.

Backwards compatibility is scoped to a major version release. New majors is the only time we'll break things unless we absolutely need to for security purposes in a minor or a patch. We need a window of opportunity to be able to break things somewhere during release cycles regardless of SOLID principles lol.

@l0gicgate If you follow SOLID principles then you are certainly right. And it is your package - and it is up to you to decide what to do. But let me ask a philosophical question that I ponder from time to time: if the initial release didn't provide for flexibility in method operation, does it mean that we should be hostage to SOLID? For example, a person forgets to include some useful thing in a method, and after the release thinks, "Yes I forgot, but SOLID doesn't allow changes."

It's a good question, I think that it's difficult to think of all use cases in user space when you're building something in isolation unless you have unlimited time to plan and execute. I do believe that in this case extending the original class is a better solution though. I'm not saying you're wrong and I'm right with this approach, this is obviously a subjective matter.

But as I mentioned in my original comment, adding nested logic to make one method do two things isn't something I'm a big fan of, also loose typing of parameters is usually a pretty big no-no for me as well.

Hopefully that answers your question

[solventt]

@l0gicgate Since we started talking about SOLID, it would be a good idea to pay attention to slimphp/Slim-Csrf that violates the first principle (single responsibility). That package includes only one class that does a bunch of things: it processes the request according to PSR-15, generates and validates tokens, handles token storage. If you look at the corresponding Symfony (https://github.com/symfony/security-csrf) or Yii framework (https://github.com/yiisoft/csrf) packages, there is a separation of responsibilities: token generation, token, token storage, token managing (e.g., middleware).

Moreover, slimphp/Slim-Csrf has a known vulnerability (exploited when the same token is returned for every request).

[solventt]

@l0gicgate You wrote: "...also loose typing of parameters is usually a pretty big no-no for me as well".

But In Slim's code I noticed:

* @param string|string[] $typeOrTypes Exception/Throwable name.                      // a couple of types
* ie: RuntimeException::class or an array of classes
* ie: [HttpNotFoundException::class, HttpMethodNotAllowedException::class]
* @param string|callable|ErrorHandlerInterface $handler                              // several types
* @param bool $handleSubclasses
* @return self
*/
public function setErrorHandler($typeOrTypes, $handler, bool $handleSubclasses = false): self
{
  if (is_array($typeOrTypes)) {
    foreach ($typeOrTypes as $type) {
      $this->addErrorHandler($type, $handler, $handleSubclasses);
    }
  } else {
    $this->addErrorHandler($typeOrTypes, $handler, $handleSubclasses);
  }

  return $this;
}