Make all middleware immutable

[shadowhand]

There are a number of configuration methods in these middleware that prevent them from being treated as immutable, for example Payload::associative which could be written as:

public function withAssociative($setting)
{
    $copy = clone $this;
    $copy->associative = (bool) $setting;
    return $copy;
}

[oscarotero]

Why should the middleware be treated as inmutable?

[shadowhand]

There's a number of articles out there about this, I'll tout one of my own: http://shadowhand.me/immutable-data-structures-in-php/ I think the best argument is:

An immutable object cannot be modified by accident.

Immutability has benefits and no (real) drawbacks.

[oscarotero]

I got the point of inmutability in psr-7, but a middleware is just a callable, not an object shared between several applications. These methods are just sugar syntax, a way to provide configuration more friendly than using an associative array with settings.

I don't see benefits (or the need) of implementing inmutability here. But maybe I'm missing an use case to prove otherwise

[schnittstabil]

Immutability also improves reusability, e.g. in Slim:

$auth = Middleware::BasicAuthentication([
    'username1' => 'password1',
    'username2' => 'password2'
]);

$app->get('/', function ($req, $res) {
    // ...
})->add($auth->realm('Index Realm'));

$app->get('/login', function ($req, $res) {
    // ...
})->add($auth->realm('Login Realm'));

[wolfy-j]

Reusability looks cool, but method signatures (with~) has to be updated which can be non-BC change.

[oscarotero]

There're many middlewares that immutability is confusing or problematic, mainly those that require other classes to work. For example, auraRouter, fastRoute or LeagueRoute: should the router instances be cloned? And whoops? Cors?, etc... To reuse BasicAuthentication, for example, you can simply do this:

$app->add((clone $auth)->realm('Index Realm'));

Or configure your container or DI class to return a new instance each time:

$app->add($container->get('middleware:auth')->realm('Index Realm'));

[shadowhand]

Typically only the thing being modified is cloned in the new instance. Collaborators are not.

[oscarotero]

Ok, a simple way to add immutability without BC, is using a magic method:

    public function __call($name, $arguments)
    {
        if (stripos($name, 'with') === 0) {
            $method = substr($name, 4);

            if (method_exists($this, $method)) {
                $clone = clone $this;
                return call_user_func_array([$clone, $method], $arguments);
            }
        }

        throw new \BadMethodCallException(sprintf('Call to undefined method "%s"', $name));
    }

What do you think?

[schnittstabil]

@oscarotero As you already mentioned, things will become confusing/problematic, e.g. with Csp:

$csp = Middleware::csp($directives)
        ->withaddSource('img-src', 'https://ytimg.com'); // not immutable

This also shows that clone $csp is a bad idea, because as a programmer I don't know if $csp->csp is already instantiated or not.

Personally, I'm fine with the current approach, mainly because I almost always use DI.

[oscarotero]

Ok, due the diversity of the middleware pieces, implementing immutability brings more problems than benefits, so it's better to leave the things as they are.

[shadowhand]

Whoa whoa... that's a hasty conclusion... why wouldn't this be done as part of the next major release?

[oscarotero]

Well, I can leave this issue opened for further discussion, waiting for a way to implement immutability clearly and intuitively. But I still think that immutability is not necessary here.

[designermonkey]

Just throwing my perspective in here (not that it is important at all).

From what I've learned about OOP design, immutability is only useful (and IMO a required practice) for business objects, entities, and their respective value objects, and data transfer objects; basically, anything that carries data over application boundaries. In that case, It would be expected that something like PSR's Request and Response interfaces are immutable.

When it comes to middlewares however, they are better defined as functional objects that interact with data to change it's state for other functional objects, and should not be immutable. In this case, immutability is more of a hindrance than a benefit.

The argument, albeit well argued, that the 'settings' of these middlewares can be edited can be addressed with a concern towards application design: If the application allows middleware or other functionality to alter settings on running middleware, then there is a boundary problem in the design of the application, and it is that which should be addressed rather than calling for immutability on functional objects.

@shadowhand I've actually read your article before this (and follow your work too), and it backs up my point, as you titled the article yourself:

Immutable Data Structures

Middleware is not a data structure, it is a functional object.

To tackle the argument about re-use that @schnittstabil makes: Yes, re-use is cool but is not the point of immutability. If you need to re-use object's with altered functionality, you should instantiate a new instance with either method that @oscarotero alluded to.

I hope I've not stirred the pot here, and hopefully added thought about this from an application design perspective.

[shadowhand]

@designermonkey I think you make a good argument. Personally I don't see a downside to making everything immutable, but you're likely right that there is no particular benefit in this scenario.

[oscarotero]

I'm agree with @designermonkey so I'll close this. Thanks everybody for the discussion. I've learned a lot.