allow method chaining

[jens1o]

Personally, I'd like to chain my settings, so instead of this:

// init SecureHeaders
$headers = new SecureHeaders;

$headers->auto();
$headers->hsts();
$headers->csp([
    'default-src' => [
        'none'
    ],
    'script-src' => [
        'self'
    ],
    'style-src' => [
        'self',
        'https://fonts.googleapis.com'
    ],
    'img-src' => [
        'https://www.gravatar.com'
    ]
]);
$headers->apply();

I could do

// init SecureHeaders
$headers = new SecureHeaders;

$headers
    ->auto()
    ->hsts()
    ->csp([
        'default-src' => [
            'none'
        ],
        'script-src' => [
            'self'
        ],
        'style-src' => [
            'self',
            'https://fonts.googleapis.com'
        ],
        'img-src' => [
            'https://www.gravatar.com'
        ]
    ])
    ->apply();

[aidantwoods]

Would you not find it confusing when using methods that cannot return the current instance?

E.g. https://github.com/aidantwoods/SecureHeaders/wiki/cspNonce

[aidantwoods]

Also just as a btw, no need to call ->auto if you're not passing it any arguments :)

See: https://github.com/aidantwoods/SecureHeaders/wiki/auto

[...] If unconfigured, the default setting is SecureHeaders::AUTO_ALL.

[jens1o]

No, I wouldn't as soon as it's clear that this is a getter, so I would call it getCspNonce(...$args) instead.

In fact, I'm that used to it, and my first tries were to do that, and it failed. :/

But, thanks for your heads up!

[aidantwoods]

No, I wouldn't as soon as it's clear that this is a getter, so I would call it getCspNonce(...$args) instead.

Hmm... it's not a "pure" getter though (as-in: it has side effects). It'll generate and insert the nonce into the CSP, then return the nonce so you can use it (or if one was previously generated it'll just return it). I built it like that so you don't have to care whether one already existed right when you need it.

I mention that, because I'm not sure if it would be misleading to prefix it with get since it causes changes? (And it isn't really a getter – just happens to return a value, and sometimes retrieve a stored one).

In general I'm not opposed to method chaining though, just don't want to end up causing confusing by partially adding it 😜 (i.e. is unpredictable usability worse than not having it?)

[jens1o]

Well, then I leave the choice up to you, since you are the maintainer. :P

It's good to say no. :D

[aidantwoods]

I think it'll have to be a no for now. I may revisit in future though.

---

As an aside: just some thoughts on method chaining, having a think about it ;p

There appear to be two design patterns that use it:

1. Where you deliberately act on very different objects returned in a "flow" like way, e.g.

   $boo = $App->getConfig()->getId('Foo.Bar.Baz')->getValue();

2. For configuration, saving typing the original object multiple times

   $FooBar
       ->loadConfig($Config)
       ->loadExtensions($Extensions)
       ->run();

For the former, it relies on you knowing what is returned, and makes a nice way of using what was returned easily.

For the latter, the actual method itself returning the instance seems a bit useless as actual information (unless dealing with immutable objects – where it in-fact returns a new instance. But that's really just the former case – since the method was designed with returning something in mind anyway).

So I wonder whether for the latter case, it would almost be better to implement that kind of method chaining as a language feature (instead of doing it via a return statement that doesn't have anything to do with the method's role).

E.g. an &> operator, like -> but meaning something like "call foo and bar on this object".

So instead of the author of the object implementing support for you to do this (on methods that would otherwise have no reason to return anything). Or you could even use it on things that do return things (if you don't care about what's returned).

I.e. write this (knowing that it would work: because language feature).

$Baz
  &>foo()
  &>bar();

Practically, I guess it could act to call the method like -> would do, but ignore any return statement and pass on the object on the LHS. That way it would be clear when you're acting on the same object from a readers point of view too.

E.g. it would allow you to write:

$headers
    &>hsts()
    &>csp([
        'default-src' => [
            'none'
        ],
        'script-src' => [
            'self'
        ],
        'style-src' => [
            'self',
            'https://fonts.googleapis.com'
        ],
        'img-src' => [
            'https://www.gravatar.com'
        ]
    ])
    &>apply();

without the underlying library actually changing any code.

Let me know if that doesn't sound too insane – could perhaps send a message round on PHP internals to see if an RFC for something like this would be worth doing.

[jens1o]

Okay. I'm fine with this, even though I'd like to be able to chain methods. :)

Please write an RFC, that's a pretty good idea and it really cleans up the code!

[franzliedke]

Interesting discussion here. 😄

I wish I would have seen this a bit earlier. To be honest, I think it's not the best style to have this method behave the way it does. The mix of setting and getting stuff might seem comfortable, but is hard to predict (the side effect determined by another flag on the class makes matters even worse).

Can you provide an example where the consumer of this library would not be aware of having added a nonce already, and would thus need protection from doing so? IMO, that is something consumers should have to take care of on their own.

Since the SecureHeaders class is mostly about configuration now, having it return itself (or even a new instance, thus becoming immutable) on these setter methods would be very nice.

[aidantwoods]

I can definely agree with it not being terribly nice :p

Perhaps this should be split into seperate methods to generate + add, and then subsequently get the nonce?

[aidantwoods]

@jens1o

Please write an RFC, that's a pretty good idea and it really cleans up the code!

I did send something round on internals, appears to have been missed/not replied to anyway – though feel free to reply to try get some discussion going 😉
https://externals.io/message/100007

PHP rules say I can ask for RFC writing permissions if feedback is not negative, pretty sure no feedback meets that criteria, right? ;p

[jens1o]

Hmm, is it just me or can't the public reply to it?

[aidantwoods]

Link is to a read only mirror of the mailing list (not really another good way to show it). I can probably forward you the original email, I think you'd need to subscribe to reply to the internals list though – not personally a fan of the whole mailing list setup for any discussion :p

http://php.net/mailing-lists.php

[aidantwoods]

Reopening this. My thinking is to create a configuration object that can be passed to SecureHeaders in the next release, if we separate config from the other functionality I see no reason not to support method chaining for the config object.