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.

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

Update bundle for Sylius 1.9 / Symfony 4.4+||5.2+ #251

Merged
merged 30 commits into from
Mar 27, 2021
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
008f60f
changed package name
Gogopex Mar 5, 2021
6c0b403
stopwatch version fix
Gogopex Mar 5, 2021
b703b22
updated the treebuilder in Configuration.php and replaced ObjectMappe…
Gogopex Mar 6, 2021
3ca8fd7
Update src/DependencyInjection/Configuration.php
Gogopex Mar 6, 2021
78fed76
Merge pull request #1 from MyLittleParis/sylius1.9-update
Gogopex Mar 6, 2021
91232da
fixed returns
Gogopex Mar 6, 2021
fe051e4
Merge pull request #2 from MyLittleParis/sylius1.9-update
Gogopex Mar 6, 2021
132be1a
Update composer.json
Gogopex Mar 6, 2021
b1341a9
using this bundle until FoSylius merges update PR
Gogopex Mar 7, 2021
b86fc6d
updated services.yml formatting for symf4.4+
Gogopex Mar 7, 2021
0e9cce6
travis.yml update for Sylius 1.9
Gogopex Mar 7, 2021
12d0e72
updated two additional commands to Symfony 4.4+ standard, swapped bac…
Gogopex Mar 7, 2021
b30c0fe
changed travis config, symfony dependency 5.2 to 4.4
Gogopex Mar 7, 2021
9caf4ac
updated composer.json -> replaced | by || as | is deprecated -> bumpe…
Gogopex Mar 7, 2021
1bb891d
remove PHP 7.2 pipeline for Sylius 1.9 as it only supports ^7.3
Gogopex Mar 7, 2021
b42dfa3
updated composer.json for sylius 1.9
Gogopex Mar 7, 2021
2c6b206
Merge pull request #3 from MyLittleParis/composer-updates
Gogopex Mar 7, 2021
cd9dcc5
sylius-labs/coding-standard update
Gogopex Mar 7, 2021
5fc1c4c
Merge pull request #4 from MyLittleParis/composer-updates-2
Gogopex Mar 7, 2021
daf5f9a
additional bundle updates
Gogopex Mar 7, 2021
4033c8c
Merge pull request #5 from MyLittleParis/composer-updates-2
Gogopex Mar 7, 2021
c1f0b43
added int to return
Gogopex Mar 7, 2021
5cc5a7c
replaced ObjectManager typing by EntityManagerInterface as it has bee…
Gogopex Mar 7, 2021
5b70d47
Update src/DependencyInjection/Configuration.php
Gogopex Mar 7, 2021
922ebf0
github action PoC from sylius invoicing plugin (#6)
Gogopex Mar 15, 2021
a667967
GitHub action (#8)
Gogopex Mar 15, 2021
945881d
GitHub action (#9)
Gogopex Mar 24, 2021
65e0c9d
GitHub action (#10)
Gogopex Mar 25, 2021
8ad9c4d
Object manager typing (#11)
Gogopex Mar 27, 2021
f40511a
revert EntityManagerInterface typing
Gogopex Mar 27, 2021
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
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"php": "^7.2",
"sylius/sylius": "^1.4",
"portphp/portphp": "^1.2",
"symfony/stopwatch": "^3.3 | ^4.1 | ^5.0",
"symfony/stopwatch": "^4.4 || ^5.2",
"queue-interop/queue-interop": "^0.6.2|^0.7|^0.8"
},
"suggest": {
Expand Down
14 changes: 10 additions & 4 deletions src/Command/ExportDataCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,27 @@ protected function configure(): void
new InputArgument('file', InputArgument::OPTIONAL, 'The target file to export to.'),
new InputOption('format', null, InputOption::VALUE_REQUIRED, 'The format of the file to export to'),
/** @todo Extracting details to show with this option. At the moment it will have no effect */
new InputOption('details', null, InputOption::VALUE_NONE,
'If to return details about skipped/failed rows'),
new InputOption(
'details',
null,
InputOption::VALUE_NONE,
'If to return details about skipped/failed rows'
),
]);
}

/**
* {@inheritdoc}
*/
protected function execute(InputInterface $input, OutputInterface $output): void
protected function execute(InputInterface $input, OutputInterface $output): int
{
/** @var string $exporter */
$exporter = $input->getArgument('exporter');

if (empty($exporter)) {
$this->listExporters($input, $output);

return;
return 0;
}

$format = $input->getOption('format');
Expand Down Expand Up @@ -97,6 +101,8 @@ protected function execute(InputInterface $input, OutputInterface $output): void
$file,
$name
));

return 0;
}

private function listExporters(InputInterface $input, OutputInterface $output, ?string $errorMessage = null): void
Expand Down
12 changes: 9 additions & 3 deletions src/DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,14 @@ final class Configuration implements ConfigurationInterface
*/
public function getConfigTreeBuilder(): TreeBuilder
{
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('fos_sylius_import_export');
if (method_exists(TreeBuilder::class, 'getRootNode')) {
Gogopex marked this conversation as resolved.
Show resolved Hide resolved
$treeBuilder = new TreeBuilder('fos_sylius_import_export');
$rootNode = $treeBuilder->getRootNode();
} else {
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('fos_sylius_import_export');
}


$rootNode
->children()
Expand Down Expand Up @@ -43,7 +49,7 @@ public function getConfigTreeBuilder(): TreeBuilder
->end()
->end()
->end()
->end();
->end();
Gogopex marked this conversation as resolved.
Show resolved Hide resolved
Gogopex marked this conversation as resolved.
Show resolved Hide resolved

return $treeBuilder;
}
Expand Down
4 changes: 2 additions & 2 deletions src/Importer/JsonResourceImporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@

namespace FriendsOfSylius\SyliusImportExportPlugin\Importer;

use Doctrine\Common\Persistence\ObjectManager;
use Doctrine\ORM\EntityManagerInterface;
use FriendsOfSylius\SyliusImportExportPlugin\Exception\ImporterException;
use FriendsOfSylius\SyliusImportExportPlugin\Processor\ResourceProcessorInterface;

final class JsonResourceImporter extends ResourceImporter implements SingleDataArrayImporterInterface
{
public function __construct(
ObjectManager $objectManager,
EntityManagerInterface $objectManager,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was this change needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ie. why the switch to the ORM specific interface

Copy link
Contributor Author

@Gogopex Gogopex Mar 7, 2021

Choose a reason for hiding this comment

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

I was encountering an error in both importers when trying to run any export/import commands, something of the sort

Fatal Error: 
Argument 1 passed to XXX must be an instance of Doctrine\Common\Persistence\ObjectManager, instance of ContainerNrPkzm2\EntityManager_9a5be93 given

The short version is that to circumvent this, I changed these two typehints to reflect the object that was being injected 😄

The long version: I scratched my head at this for a while as

  • ObjectManager is an interface
  • EntityManagerInterface itself extends ObjectManager
    But there seemed to be shenanigans I did not understand with what is being injected. ContainerNrPkzm2\EntityManager_9a5be93 itself seems to implement the wrong thing.

Some source: https://stackoverflow.com/a/45678423/2926899

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can do 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to typehint Doctrine\Persistence\ObjectManager, I don't think I've ever seen the deprecation notice before where it specifically calls out to use the ORM's interface TBH. But, I did just monkey patch an app I'm trying to finish upgrading to Sylius 1.9 with the persistence namespace change and had zero issue using the base ObjectManager.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this remains the only concern I have left over merging this PR .. I have reached out to the Symfony community for advice once more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have a test case that fails if we revert this change? if not, could we add one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

“ You’re using the persistence interface to autowire the entity manager. In future versions of DoctrineBundle (next major release) you’ll have to either alias this yourself or, better yet, inject the manager registry”

https://symfony-devs.slack.com/archives/C3FQPE6LE/p1616851875129800?thread_ts=1616793098.118500&channel=C3FQPE6LE&message_ts=1616851875.129800

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the advice you guys gave and reverted the typing and tested the import command and everything worked fine. I still think there is something there: both https://github.com/doctrine/DoctrineBundle/blob/2.2.x/UPGRADE-2.0.md#service-aliases and the fact that I did encounter the error from the original comment is strange 🤔

It would be great if someone else could give the import command a try and confirm that it does indeed work just fine with the ObjectManager typing 🙏

tl;dr it works just fine with the ObjectManager typing, as @mbabker implied 👍

ResourceProcessorInterface $resourceProcessor,
ImportResultLoggerInterface $importerResult,
int $batchSize,
Expand Down
8 changes: 4 additions & 4 deletions src/Importer/ResourceImporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@

namespace FriendsOfSylius\SyliusImportExportPlugin\Importer;

use Doctrine\Common\Persistence\ObjectManager;
use Port\Reader\ReaderFactory;
use Doctrine\ORM\EntityManagerInterface;
use FriendsOfSylius\SyliusImportExportPlugin\Exception\ImporterException;
use FriendsOfSylius\SyliusImportExportPlugin\Exception\ItemIncompleteException;
use FriendsOfSylius\SyliusImportExportPlugin\Processor\ResourceProcessorInterface;
use Port\Reader\ReaderFactory;

class ResourceImporter implements ImporterInterface
{
/** @var ReaderFactory */
private $readerFactory;

/** @var ObjectManager */
/** @var EntityManagerInterface */
protected $objectManager;

/** @var ResourceProcessorInterface */
Expand All @@ -38,7 +38,7 @@ class ResourceImporter implements ImporterInterface

public function __construct(
ReaderFactory $readerFactory,
ObjectManager $objectManager,
EntityManagerInterface $objectManager,
Gogopex marked this conversation as resolved.
Show resolved Hide resolved
ResourceProcessorInterface $resourceProcessor,
ImportResultLoggerInterface $importerResult,
int $batchSize,
Expand Down
2 changes: 1 addition & 1 deletion src/Resources/config/services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ services:

sylius.class_metadata.product_image:
class: Doctrine\ORM\Mapping\ClassMetadata
factory: 'doctrine.orm.default_entity_manager:getClassMetadata'
factory: ['@doctrine.orm.default_entity_manager' , 'getClassMetadata']
arguments: ['Sylius\Component\Core\Model\ProductImage']

sylius.repository.product_image:
Expand Down