Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable local file access in InvoicePdfFileGenerator #272

Merged
merged 7 commits into from
Mar 31, 2022
Merged

Enable local file access in InvoicePdfFileGenerator #272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c8a8aee
Empty options in KnpSnappy pdf configuration
coldic3 Mar 28, 2022
91070d5
Make allowed files configurable for PDF generator
coldic3 Mar 28, 2022
1eeda81
Add TwigToPdfGenerator adapter
coldic3 Mar 28, 2022
0b0d548
Update UPGRADE.md
coldic3 Mar 30, 2022
b640c6b
Extend KnpSnappy pdf options instead of overriding them
coldic3 Mar 30, 2022
47b8a1c
Add PdfOptionsGenerator in place of TwigToPdfGenerator
coldic3 Mar 31, 2022
264b909
Update UPGRADE.md
coldic3 Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
### UPGRADE FROM 0.20.0 TO 0.21.0

1. `Sylius\InvoicingPlugin\Generator\InvoicePdfFileGenerator` takes `Sylius\InvoicingPlugin\Generator\PdfOptionsGeneratorInterface` as the third argument after `Knp\Snappy\GeneratorInterface`.

### UPGRADE FROM 0.19.0 TO 0.20.0

1. Since 0.20.0, the recommended Sylius version to use with InvoicingPlugin is `1.11.*`. If you would like to upgrade Sylius to v1.11.0,
Expand Down
19 changes: 17 additions & 2 deletions spec/Generator/InvoicePdfFileGeneratorSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Sylius\InvoicingPlugin\Entity\InvoiceInterface;
use Sylius\InvoicingPlugin\Generator\InvoiceFileNameGeneratorInterface;
use Sylius\InvoicingPlugin\Generator\InvoicePdfFileGeneratorInterface;
use Sylius\InvoicingPlugin\Generator\PdfOptionsGeneratorInterface;
use Sylius\InvoicingPlugin\Model\InvoicePdf;
use Symfony\Component\Config\FileLocatorInterface;
use Twig\Environment;
Expand All @@ -28,12 +29,14 @@ final class InvoicePdfFileGeneratorSpec extends ObjectBehavior
function let(
Environment $twig,
GeneratorInterface $pdfGenerator,
PdfOptionsGeneratorInterface $pdfOptionsGenerator,
FileLocatorInterface $fileLocator,
InvoiceFileNameGeneratorInterface $invoiceFileNameGenerator
): void {
$this->beConstructedWith(
$twig,
$pdfGenerator,
$pdfOptionsGenerator,
$fileLocator,
$invoiceFileNameGenerator,
'invoiceTemplate.html.twig',
Expand All @@ -50,6 +53,7 @@ function it_creates_invoice_pdf_with_generated_content_and_filename_basing_on_in
FileLocatorInterface $fileLocator,
Environment $twig,
GeneratorInterface $pdfGenerator,
PdfOptionsGeneratorInterface $pdfOptionsGenerator,
InvoiceFileNameGeneratorInterface $invoiceFileNameGenerator,
InvoiceInterface $invoice,
ChannelInterface $channel
Expand All @@ -64,8 +68,19 @@ function it_creates_invoice_pdf_with_generated_content_and_filename_basing_on_in
->willReturn('<html>I am an invoice pdf file content</html>')
;

$pdfGenerator->getOutputFromHtml('<html>I am an invoice pdf file content</html>')->willReturn('PDF FILE');
$pdfOptionsGenerator
->generate()
->willReturn(['allow' => ['located-path/sylius-logo.png']])
;

$pdfGenerator
->getOutputFromHtml('<html>I am an invoice pdf file content</html>', ['allow' => ['located-path/sylius-logo.png']])
->willReturn('PDF FILE')
;

$this->generate($invoice)->shouldBeLike(new InvoicePdf('2015_05_00004444.pdf', 'PDF FILE'));
$this
->generate($invoice)
->shouldBeLike(new InvoicePdf('2015_05_00004444.pdf', 'PDF FILE'))
;
}
}
52 changes: 52 additions & 0 deletions spec/Generator/PdfOptionsGeneratorSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Paweł Jędrzejewski
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace spec\Sylius\InvoicingPlugin\Generator;

use PhpSpec\ObjectBehavior;
use Sylius\InvoicingPlugin\Generator\PdfOptionsGeneratorInterface;
use Symfony\Component\Config\FileLocatorInterface;

final class PdfOptionsGeneratorSpec extends ObjectBehavior
{
function let(FileLocatorInterface $fileLocator): void
{
$this->beConstructedWith(
$fileLocator,
['allow' => 'allowed_file_in_knp_snappy_config.png'],
['swans.png']
);
}

function it_is_pdf_options_generator_interface(): void
{
$this->shouldImplement(PdfOptionsGeneratorInterface::class);
}

function it_generates_pdf_options(FileLocatorInterface $fileLocator): void
{
$fileLocator
->locate('swans.png')
->willReturn('located-path/swans.png');

$this
->generate()
->shouldBeLike([
'allow' => [
'allowed_file_in_knp_snappy_config.png',
'located-path/swans.png',
],
])
;
}
}
124 changes: 74 additions & 50 deletions src/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use Sylius\InvoicingPlugin\Factory\InvoiceShopBillingDataFactory;
use Sylius\InvoicingPlugin\Factory\LineItemFactory;
use Sylius\InvoicingPlugin\Factory\TaxItemFactory;
use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\ConfigurationInterface;

Expand All @@ -42,7 +43,15 @@ public function getConfigTreeBuilder(): TreeBuilder
$treeBuilder = new TreeBuilder('sylius_invoicing_plugin');
$rootNode = $treeBuilder->getRootNode();

$rootNode
$this->addResourcesSection($rootNode);
$this->addPdfGeneratorSection($rootNode);

return $treeBuilder;
}

private function addResourcesSection(ArrayNodeDefinition $node): void
{
$node
->children()
->arrayNode('resources')
->addDefaultsIfNotSet()
Expand Down Expand Up @@ -78,63 +87,63 @@ public function getConfigTreeBuilder(): TreeBuilder
->end()
->end()
->end()
->end()
->arrayNode('shop_billing_data')
->addDefaultsIfNotSet()
->children()
->variableNode('options')->end()
->arrayNode('classes')
->addDefaultsIfNotSet()
->children()
->scalarNode('model')->defaultValue(InvoiceShopBillingData::class)->cannotBeEmpty()->end()
->scalarNode('interface')->defaultValue(InvoiceShopBillingDataInterface::class)->cannotBeEmpty()->end()
->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end()
->scalarNode('factory')->defaultValue(InvoiceShopBillingDataFactory::class)->cannotBeEmpty()->end()
->scalarNode('repository')->cannotBeEmpty()->end()
->end()
->end()
->arrayNode('shop_billing_data')
->addDefaultsIfNotSet()
->children()
->variableNode('options')->end()
->arrayNode('classes')
->addDefaultsIfNotSet()
->children()
->scalarNode('model')->defaultValue(InvoiceShopBillingData::class)->cannotBeEmpty()->end()
->scalarNode('interface')->defaultValue(InvoiceShopBillingDataInterface::class)->cannotBeEmpty()->end()
->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end()
->scalarNode('factory')->defaultValue(InvoiceShopBillingDataFactory::class)->cannotBeEmpty()->end()
->scalarNode('repository')->cannotBeEmpty()->end()
->end()
->end()
->end()
->arrayNode('line_item')
->addDefaultsIfNotSet()
->children()
->variableNode('options')->end()
->arrayNode('classes')
->addDefaultsIfNotSet()
->children()
->scalarNode('model')->defaultValue(LineItem::class)->cannotBeEmpty()->end()
->scalarNode('interface')->defaultValue(LineItemInterface::class)->cannotBeEmpty()->end()
->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end()
->scalarNode('factory')->defaultValue(LineItemFactory::class)->cannotBeEmpty()->end()
->scalarNode('repository')->cannotBeEmpty()->end()
->end()
->end()
->arrayNode('line_item')
->addDefaultsIfNotSet()
->children()
->variableNode('options')->end()
->arrayNode('classes')
->addDefaultsIfNotSet()
->children()
->scalarNode('model')->defaultValue(LineItem::class)->cannotBeEmpty()->end()
->scalarNode('interface')->defaultValue(LineItemInterface::class)->cannotBeEmpty()->end()
->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end()
->scalarNode('factory')->defaultValue(LineItemFactory::class)->cannotBeEmpty()->end()
->scalarNode('repository')->cannotBeEmpty()->end()
->end()
->end()
->end()
->arrayNode('tax_item')
->addDefaultsIfNotSet()
->children()
->variableNode('options')->end()
->arrayNode('classes')
->addDefaultsIfNotSet()
->children()
->scalarNode('model')->defaultValue(TaxItem::class)->cannotBeEmpty()->end()
->scalarNode('interface')->defaultValue(TaxItemInterface::class)->cannotBeEmpty()->end()
->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end()
->scalarNode('factory')->defaultValue(TaxItemFactory::class)->cannotBeEmpty()->end()
->scalarNode('repository')->cannotBeEmpty()->end()
->end()
->end()
->arrayNode('tax_item')
->addDefaultsIfNotSet()
->children()
->variableNode('options')->end()
->arrayNode('classes')
->addDefaultsIfNotSet()
->children()
->scalarNode('model')->defaultValue(TaxItem::class)->cannotBeEmpty()->end()
->scalarNode('interface')->defaultValue(TaxItemInterface::class)->cannotBeEmpty()->end()
->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end()
->scalarNode('factory')->defaultValue(TaxItemFactory::class)->cannotBeEmpty()->end()
->scalarNode('repository')->cannotBeEmpty()->end()
->end()
->end()
->end()
->arrayNode('invoice_sequence')
->addDefaultsIfNotSet()
->children()
->variableNode('options')->end()
->arrayNode('classes')
->addDefaultsIfNotSet()
->children()
->scalarNode('model')->defaultValue(InvoiceSequence::class)->cannotBeEmpty()->end()
->end()
->arrayNode('invoice_sequence')
->addDefaultsIfNotSet()
->children()
->variableNode('options')->end()
->arrayNode('classes')
->addDefaultsIfNotSet()
->children()
->scalarNode('model')->defaultValue(InvoiceSequence::class)->cannotBeEmpty()->end()
->scalarNode('interface')->defaultValue(InvoiceSequenceInterface::class)->cannotBeEmpty()->end()
->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end()
->scalarNode('factory')->defaultValue(Factory::class)->cannotBeEmpty()->end()
Expand All @@ -147,7 +156,22 @@ public function getConfigTreeBuilder(): TreeBuilder
->end()
->end()
;
}

return $treeBuilder;
private function addPdfGeneratorSection(ArrayNodeDefinition $node): void
{
$node
->children()
->arrayNode('pdf_generator')
->addDefaultsIfNotSet()
->children()
->arrayNode('allowed_files')
->useAttributeAsKey('name')
->variablePrototype()->end()
->end()
->end()
->end()
->end()
;
}
}
2 changes: 2 additions & 0 deletions src/DependencyInjection/SyliusInvoicingExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public function load(array $config, ContainerBuilder $container): void
$this->registerResources('sylius_invoicing_plugin', 'doctrine/orm', $config['resources'], $container);

$loader->load('services.xml');

$container->setParameter('sylius_invoicing.pdf_generator.allowed_files', $config['pdf_generator']['allowed_files']);
}

public function prepend(ContainerBuilder $container): void
Expand Down
34 changes: 9 additions & 25 deletions src/Generator/InvoicePdfFileGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,32 +21,15 @@

final class InvoicePdfFileGenerator implements InvoicePdfFileGeneratorInterface
{
private Environment $templatingEngine;

private GeneratorInterface $pdfGenerator;

private FileLocatorInterface $fileLocator;

private InvoiceFileNameGeneratorInterface $invoiceFileNameGenerator;

private string $template;

private string $invoiceLogoPath;

public function __construct(
Environment $templatingEngine,
GeneratorInterface $pdfGenerator,
FileLocatorInterface $fileLocator,
InvoiceFileNameGeneratorInterface $invoiceFileNameGenerator,
string $template,
string $invoiceLogoPath
private Environment $templatingEngine,
private GeneratorInterface $pdfGenerator,
private PdfOptionsGeneratorInterface $pdfOptionsGenerator,
private FileLocatorInterface $fileLocator,
private InvoiceFileNameGeneratorInterface $invoiceFileNameGenerator,
private string $template,
private string $invoiceLogoPath
) {
$this->templatingEngine = $templatingEngine;
$this->pdfGenerator = $pdfGenerator;
$this->fileLocator = $fileLocator;
$this->invoiceFileNameGenerator = $invoiceFileNameGenerator;
$this->template = $template;
$this->invoiceLogoPath = $invoiceLogoPath;
}

public function generate(InvoiceInterface $invoice): InvoicePdf
Expand All @@ -58,7 +41,8 @@ public function generate(InvoiceInterface $invoice): InvoicePdf
'invoice' => $invoice,
'channel' => $invoice->channel(),
'invoiceLogoPath' => $this->fileLocator->locate($this->invoiceLogoPath),
])
]),
$this->pdfOptionsGenerator->generate()
);

return new InvoicePdf($filename, $pdf);
Expand Down
48 changes: 48 additions & 0 deletions src/Generator/PdfOptionsGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Paweł Jędrzejewski
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Sylius\InvoicingPlugin\Generator;

use Symfony\Component\Config\FileLocatorInterface;

final class PdfOptionsGenerator implements PdfOptionsGeneratorInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about this naming?

Suggested change
final class PdfOptionsGenerator implements PdfOptionsGeneratorInterface
final class PdfOptionsWithAllowedFilesProvider implements PdfOptionsProviderInterface

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you go with "pdf options provider interface", then I would keep the interface name as an integral suffix, i.e.: "allowed files pdf options provider". As an extra improvement, I would move the "pdf" from class name to namespace.

{
public function __construct(
private FileLocatorInterface $fileLocator,
private array $knpSnappyOptions,
private array $allowedFiles
) {
}

public function generate(): array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you would follow naming change:

Suggested change
public function generate(): array
public function provide(): array

{
$options = $this->knpSnappyOptions;

if (empty($this->allowedFiles)) {
return $options;
}

if (!isset($options['allow'])) {
$options['allow'] = [];
} elseif (!is_array($options['allow'])) {
$options['allow'] = [$options['allow']];
}

$options['allow'] = array_merge(
$options['allow'],
array_map(fn ($file) => $this->fileLocator->locate($file), $this->allowedFiles)
);

return $options;
Comment on lines +35 to +46
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a style suggestion:

Suggested change
if (!isset($options['allow'])) {
$options['allow'] = [];
} elseif (!is_array($options['allow'])) {
$options['allow'] = [$options['allow']];
}
$options['allow'] = array_merge(
$options['allow'],
array_map(fn ($file) => $this->fileLocator->locate($file), $this->allowedFiles)
);
return $options;
$locatedFiles = array_map(fn ($file) => $this->fileLocator->locate($file), $this->allowedFiles);
if (!isset($options['allow'])) {
return $locatedFiles;
}
if (!is_array($options['allow'])) {
$options['allow'] = [$options['allow']];
}
$options['allow'] = array_merge(
$options['allow'],
$locatedFiles
);
return $options;

However, some cases are not covered by this class spec. For sure we do not cover the absence of $options['allow']. The case with $options['allow'] as the array is not covered as well. When can it happen?

It is not the case for spec probably, but what will happen if the file does not exist? What is returned by the fileLocator in such a case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now $options['allow'] cannot be an array but it could be possible in the future because it makes sense so I have put this if statement to cover that case anyway 🤔

File locator throws an error for unexisting files.

}
}
Loading