Poc/builder v2 #118
Poc/builder v2 #118
Conversation
Jean-Beru
force-pushed
the
poc/builder_v2
branch
2 times, most recently
from
86de3d0 to
5827398
Compare
|
|
||
| protected function getUrlGenerator(): UrlGeneratorInterface | ||
| { | ||
| if (!($urlGenerator = $this->dependencies->get('urlGenerator')) instanceof UrlGeneratorInterface) { |
There was a problem hiding this comment.
| if (!($urlGenerator = $this->dependencies->get('urlGenerator')) instanceof UrlGeneratorInterface) { | |
| if (!($urlGenerator = $this->dependencies->get('router')) instanceof UrlGeneratorInterface) { |
?
| service('sensiolabs_gotenberg.asset.base_dir_formatter'), | ||
| service_locator([ | ||
| 'asset_base_dir_formatter' => service('sensiolabs_gotenberg.asset.base_dir_formatter'), | ||
| 'logger' => service('logger')->nullOnInvalid(), |
There was a problem hiding this comment.
| $assets = $this->getBodyBag()->get('assets', []); | ||
|
|
||
| if (\array_key_exists($path, $assets)) { | ||
| return $this; |
There was a problem hiding this comment.
should we log a warning ? Also I think we should check if two files have the same name. I think gotenberg treat them as same file.
There was a problem hiding this comment.
We could, but I don't think that it will create useless logs when your Twig template use gotenberg_asset multiple times. For example, a logo displayed in the first and the last page.
There was a problem hiding this comment.
you are right. maybe if both the filename is the same but the path is different
There was a problem hiding this comment.
AFAIK, we can only use files under the root folder. It could be possible but we'll have to generate a unique ID depending on the file path when adding (and using) the asset.
|
|
||
| return $this; | ||
| } | ||
| } |
|
|
||
| $current = $this->getBodyBag()->get('extraHttpHeaders', []); | ||
|
|
||
| $this->getBodyBag()->set('extraHttpHeaders', array_merge($current, $headers)); |
There was a problem hiding this comment.
what about using an object like arrayiterator to always have a reference to push elements into ? Might be better than array_merge. Would also require normalizer to convert it to array
There was a problem hiding this comment.
Good idea. Would it be possible to replace an existing header with it?
There was a problem hiding this comment.
Normally yes. I don't see why not.
| */ | ||
| public function emulatedMediaType(EmulatedMediaType $mediaType): static | ||
| { | ||
| $this->getBodyBag()->set('emulatedMediaType', $mediaType); |
There was a problem hiding this comment.
->value is missing or am I missing something ? (allowedValues)
- EOF
| { | ||
| use DependencyAwareTrait; | ||
|
|
||
| protected function getAssetBaseDirFormatter(): AssetBaseDirFormatter |
There was a problem hiding this comment.
should maybe directly be part of DependencyAwareTrait. WDYT ?
There was a problem hiding this comment.
Will this dependency always be required ? Maybe the read metadata API will be the only API with no files form data 🤔
There was a problem hiding this comment.
Clearly not. But I think it does not clutter too much the builder files and simplify things a lot.
|
|
||
| protected function getLogger(): LoggerInterface|null | ||
| { | ||
| if (($logger = $this->dependencies->get('logger')) instanceof Environment) { |
There was a problem hiding this comment.
| if (($logger = $this->dependencies->get('logger')) instanceof Environment) { | |
| if (($logger = $this->dependencies->get('logger')) instanceof LoggerInterface) { |
| namespace Sensiolabs\GotenbergBundle\Builder\Behaviors\Dependencies; | ||
|
|
||
| use Psr\Log\LoggerInterface; | ||
| use Twig\Environment; |
There was a problem hiding this comment.
| use Twig\Environment; |
| { | ||
| protected function createBuilder(GotenbergClientInterface $client, ContainerInterface $dependencies): BuilderInterface | ||
| { | ||
| $dependencies->set('asset_base_dir_formatter', new AssetBaseDirFormatter(new Filesystem(), __DIR__, 'fixtures')); |
There was a problem hiding this comment.
Could be mocked if AssetBaseDirFormatter is reworked (final and without interface for now)
src/Builder/AbstractBuilder.php
Outdated
| { | ||
| $response = $this->client->call($this->getEndpoint(), $this->bodyBag, $this->headersBag); | ||
|
|
||
| if (!\in_array($response->getStatusCode(), [200, 204], true)) { |
There was a problem hiding this comment.
hmmm I don't see the benefit of having this here instead of in the call method. I was thinking more in the GileResult object. WDYT ?
Jean-Beru
force-pushed
the
poc/builder_v2
branch
4 times, most recently
from
9bdb6f7 to
fdc00a1
Compare
Jean-Beru
force-pushed
the
poc/builder_v2
branch
from
fdc00a1 to
95ba735
Compare
WIP with limited feature and a lot of old/useless files