[RELEASE-V8] Add new fields #33
Conversation
StevenRenaux
requested review from
Neirda24 and
Jean-Beru
and removed request for
Neirda24
| * | ||
| * @see https://gotenberg.dev/docs/routes#cookies-chromium | ||
| */ | ||
| public function addCookies(array $cookies): static |
There was a problem hiding this comment.
addCookie or just cookie ?
There was a problem hiding this comment.
As addExtraHttpHeader you can add cookies in addition to the ones in the configuration file if needed.
And if not use cookies only.
There was a problem hiding this comment.
you can have a look about the doc where I explain that 😉 Let me know what you think
https://github.com/sensiolabs/GotenbergBundle/pull/33/files#diff-dbcb3d682e3e3080ba4b0f0195b7215457be7782b74898ccbefed9a74ba3be26R267
There was a problem hiding this comment.
So addCookies could be named addCookie since we only add one.
There was a problem hiding this comment.
I updated the method if the user wants to add several to those already loaded from their configuration file.
WDYT?
There was a problem hiding this comment.
I think it is confusing. On other classes we have singulars on add methods. I don't see why this one needs to be different. But maybe I'm just missing something.
There was a problem hiding this comment.
I thought about addExtraHttpHeaders when I created that one.
There was a problem hiding this comment.
Let me know and I can rename it ;)
There was a problem hiding this comment.
If we name them addCookies and addExtraHttpHeaders, they should accept a list. If we name them without a "s" (addCookie and addExtraHttpHeader), they should accept a single value.
PS: I don't have a strong opinion on choosing one of these two solutions
| */ | ||
| public function addCookies(array $cookies): static | ||
| { | ||
| $this->formFields['cookies'][] = $cookies; |
There was a problem hiding this comment.
should ensure cookies is already a [] with $this->formFields['cookies'] ??= [] before
…v8-add-new-fields # Conflicts: # docs/configuration.rst # docs/customization.rst # src/Builder/AbstractChromiumPdfBuilder.php # src/DependencyInjection/Configuration.php # tests/Builder/HtmlPdfBuilderTest.php # tests/DependencyInjection/ConfigurationTest.php # tests/DependencyInjection/SensiolabsGotenbergExtensionTest.php
IMO, we should avoid a new dependency with |
|
Add |
Add new form fields failOnHttpStatusCodes
Add new form fields skipNetworkIdleEvent
Add new form fields cookies