-
Notifications
You must be signed in to change notification settings - Fork 14
make HEAD/GET method rewrite configurable #20
base: main
Are you sure you want to change the base?
Conversation
|
Seems reasonable but Fred has stronger opinions about this so I'll leave it to him. Some potential minor concerns:
|
|
I'm fine with the intent of this diff, but not the bool argument; I'm fine with a shape argument, or the instance methods. The problem with static factory methods is it's not scalable if we want to add other options in the future. |
031136c to
da5fc0b
Compare
|
updated to use options shape 👍 |
|
|
||
| abstract class BaseRouter<+TResponder> { | ||
| public function __construct( | ||
| private BaseRouterOptions $options = shape('use_get_responder_for_head' => true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps ?BaseRouterOptions, and make all the fields optional?
This will be needed when a second option is added - it doesn't really matter now, but doing it would make it clearer what the 'right way' is to add one.
This would also need something like:
$this->options = shape(
'use_get_responder_for_head' => $options['use_get_responder_for_head'] ?? true
);
closes #15