preparations for symfony 4.0 #286

ElectricMaxxx · 2017-10-28T01:41:41Z

ElectricMaxxx · 2017-10-28T07:25:24Z

@dbu can we remove 5.3 support?

dbu · 2017-10-30T07:31:39Z

Command/LoadFixtureCommand.php

@@ -136,7 +135,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
        if ($dirOrFile) {
            $paths = is_array($dirOrFile) ? $dirOrFile : array($dirOrFile);
        } else {
-            /** @var $kernel KernelInterface */
+            /** @var $kernel \Symfony\Component\HttpKernel\KernelInterface */


lets keep the use statement for this. we are using the kernel in this class so the use statement gives a hint about that.

dbu · 2017-10-30T07:32:40Z

DependencyInjection/DoctrinePHPCRExtension.php

@@ -71,6 +71,7 @@ public function load(array $configs, ContainerBuilder $container)

        if (!empty($config['manager_registry_service_id'])) {
            $container->setAlias('doctrine_phpcr', new Alias($config['manager_registry_service_id']));
+            $container->getDefinition($config['manager_registry_service_id'])->setPublic(true);


why do we dynamically set this service to public? i think the manager registry service should be always public

from Symfony 3.3 on all services are private by default, that is the major reason and work for the mirgration

and i miss some more, all that i do public in: symfony-cmf/Testing#145 really hard to find :-)

But you're setting a userland service to public that the application developer might have declared private for a reason. I don't think a bundle should do that.

Instead, since you're setting an alias to that service ID anyway, always wire and use the alias for internal logic. You could even set that alias to public if you need to access it from the container directly.

I understand that, but setting it hard to public feels like having the behavior the user used to have before Symfony 3.3, or?

A now i see that i changed the wrong line ... thanks will change it.

yep, the alias should be public, but not the other service. i tried to find what this even is for, and found no single bit of documentation neither in the doctrine phpcr bundle config reference, nor in the orm bundle doc...

i think what you need to make public however is <service id="doctrine_phpcr" class="%doctrine_phpcr.class%"> in phpcr.xml. (i will do a PR to eliminate the class parameters for master (upcoming version 2) like we do everywhere else)

I understand that, but setting it hard to public feels like having the behavior the user used to have before Symfony 3.3, or?

Private services have existed ever since. You don't know whether a service is private due to the changed defaults in Symfony 4 or because it was set to private explicitly. It's not your responsibility to create BC for other people's services.

dbu

just some nitpicks, otherwise looks good to me, thanks!

dbu · 2017-10-30T07:34:29Z

ManagerRegistry.php

+        $defaultEntityManager,
+        $proxyInterfaceName
+    ) {
+        $this->container = $container;


should we not also declare the field for this?

dbu · 2017-10-30T07:35:03Z

ManagerRegistry.php

+        array $connections,
+        array $entityManagers,
+        $defaultConnection,
+        $defaultEntityManager,


we should append Name to this and the $defaultConnection and then we can remove the docblock as it will not provide any additional information.

dbu · 2017-10-30T07:35:58Z

Resources/config/phpcr.xml

            <argument>%doctrine_phpcr.sessions%</argument>
            <argument>%doctrine_phpcr.odm.document_managers%</argument>
            <argument>%doctrine_phpcr.default_session%</argument>
            <argument>%doctrine_phpcr.odm.default_document_manager%</argument>
            <argument>%doctrine_phpcr.proxy.class%</argument>
-            <call method="setContainer">


indeed, dunno why we did it like this and not always inject the container in the constructor... thanks!

This is needed to go on symfony 4.0 i think also needed for PSR-11

As I wrote in #285, what about using a ServiceLocator (introduced in Symfony 3.3)? It may be harder to implement as you have to support both setContainer and the ServiceLocator but it looks more Symfonish!

But how to test it? I can inject a defined set of services the locator could serve right?

lets keep it simple here to keep working as it did before. service locator can be tried as a separate pull request once this is merged. ok?

derrabus · 2017-10-30T15:56:30Z

DependencyInjection/DoctrinePHPCRExtension.php

@@ -71,6 +71,7 @@ public function load(array $configs, ContainerBuilder $container)

        if (!empty($config['manager_registry_service_id'])) {
            $container->setAlias('doctrine_phpcr', new Alias($config['manager_registry_service_id']));
+            $container->getDefinition($config['manager_registry_service_id'])->setPublic(true);


But you're setting a userland service to public that the application developer might have declared private for a reason. I don't think a bundle should do that.

Instead, since you're setting an alias to that service ID anyway, always wire and use the alias for internal logic. You could even set that alias to public if you need to access it from the container directly.

derrabus · 2017-10-30T16:05:44Z

ManagerRegistry.php

 use Symfony\Bridge\Doctrine\ManagerRegistry as BaseManagerRegistry;
 use Doctrine\ODM\PHPCR\PHPCRException;

 class ManagerRegistry extends BaseManagerRegistry
 {
    /**
+     * @var ContainerInterface
+     */
+    private $container;


The ManagerRegistry class of DoctrineBridge already defines this field!

dbu · 2017-10-31T08:19:21Z

if you rebase on master, the styleci error should be gone. i fixed it on master.

i am also doing some cleanup (like dropping obsolete php and symfony versions) in #287. hopefully we can quickly wrap up that cleanup and merge, then you can rebase here and get rid of some more things.

ElectricMaxxx · 2017-10-31T09:52:57Z

@dbu heading to https://github.com/symfony-cmf/testing/blob/0749a2791a7538a96743e8417a93300c71b2694c/src/HttpKernel/ServiceVisibilityCompilerPass.php#L27 and i think i still miss some aliases/services which make problems, can you give me a hint where to finde them?

dbu · 2017-10-31T09:53:53Z

i will give it a try tonight

ElectricMaxxx · 2017-11-02T00:25:07Z

@dbu can we have both? A service and an alias with id doctrine_phpcr? I still have failing tests on cmf cause a private service with that name is used.

dbu · 2017-11-02T07:51:10Z

the alias is only created in a special case when the user specifies their own registry in the config. the alias must be public (if that is not by default). but the doctrine_phpcr service we define ourselves must be public - its defined in one of the services xml files.

ElectricMaxxx · 2017-11-02T09:21:00Z

So, i need to get this one too and than rebase on a new master.

ElectricMaxxx · 2017-11-06T08:32:21Z

@dbu why? i would like to rebase on master now and squash

dbu · 2017-11-06T08:47:46Z

i did not actively close this PR, but i deleted my branch, and this goes against my branch and not master... i think github then automatically closes the PR.

can you recreate this as a PR against master?

dbu · 2017-11-06T11:00:23Z

.travis.yml

@@ -1,6 +1,8 @@
 language: php

 php:
+  - 5.6
+  - 7.0


those where dropped on purpose, lets not re-add them

dbu · 2017-11-06T11:00:44Z

.travis.yml

      env: SYMFONY_VERSION=3.3.*
+    - php: hhvm


the php requirement 7.1 is not compatible with hhvm according to composer

dbu · 2017-11-06T11:01:08Z

composer.json

@@ -29,7 +29,7 @@
        "phpunit/php-code-coverage": "~2.2",
        "symfony/form": "~2.3|~3.0",
        "doctrine/phpcr-odm": "^1.3.0-rc4",
-        "symfony-cmf/testing": "^2.1@dev",
+        "symfony-cmf/testing": "^1.3.0",


is this on purpose?

ElectricMaxxx · 2017-11-06T11:09:10Z

@dbu the changes had been some rebase conflict waste only. Should be ok now.

dbu

just some questions about the registry, otherwise this looks fine to me!

dbu · 2017-11-06T15:36:06Z

ManagerRegistry.php

+            // and then also, the constructor should type-hint Psr\Container\ContainerInterface
+            $this->setContainer($container);
+        } else {
+            $this->container = $container;


does the new version still have the container property? but not accept the container in the constructor?

this is adopted from the orm bundle. In on case there is the property in the abstract class in the other one it lives in the trait.

dbu · 2017-11-06T15:38:16Z

ManagerRegistry.php

 use Symfony\Bridge\Doctrine\ManagerRegistry as BaseManagerRegistry;
 use Doctrine\ODM\PHPCR\PHPCRException;

 class ManagerRegistry extends BaseManagerRegistry
 {
+    public function __construct(


this is technically a BC break, but we are moving to version 2 so thats ok.

i think we should mark this class as @internal , users should just use it in the service but not as custom code.

made the class final and marked it as internal

okay, making it final does not work out. reverted that bit, only left the comment

dbu · 2017-11-06T15:38:37Z

DependencyInjection/DoctrinePHPCRExtension.php

                $connectionAliasName = sprintf('doctrine_phpcr%s.jackalope_doctrine_dbal.%s_connection', $serviceNamePrefix, $session['name']);
-                $container->setAlias($connectionAliasName, $connectionService);
+                $container->setAlias($connectionAliasName, $connectionAlias);


so much more understandable, nice cleanup!

ElectricMaxxx · 2017-11-07T12:27:52Z

So lets merge this?

dbu · 2017-11-07T12:45:57Z

ah you merged the testing component thing. great, i just squashed the commits, if its green lets merge.

dbu · 2017-11-07T12:47:12Z

.travis.yml

@@ -18,6 +18,9 @@ matrix:
    - php: 7.1
      env: SYMFONY_VERSION=3.3.*
  fast_finish: true
+  allow_failures:
+    - php: 7.1
+      env: SYMFONY_VERSION=^3.4@dev


this is not in the matrix above, so its never run. should we add it?

ElectricMaxxx · 2017-11-08T04:35:16Z

@dbu i didn't merge testing, i will do when this is merged and use ^2.0@dev there. and then i will merge content-bundle, and then ... :-)

after rebase fixes re-add missing symfony version Support Symfony 4.0

dbu · 2017-11-08T07:32:17Z

i was referring to composer.json referenced a branch of symfony-cmf/testing for a while, and now points to 2.1 again. i assumed that means you merged whatever was needed for the testing to have this merged.

thanks a lot for your efforts here!

ElectricMaxxx mentioned this pull request Oct 28, 2017

Be aware of deprecations symfony-cmf/Testing#145

Merged

ElectricMaxxx requested a review from dbu October 30, 2017 01:11

dbu reviewed Oct 30, 2017

View reviewed changes

derrabus requested changes Oct 30, 2017

View reviewed changes

dbu mentioned this pull request Oct 31, 2017

Inject Container in constructor #285

Closed

ElectricMaxxx force-pushed the prepare_for_symfony_4.0 branch from 806d295 to 3d7bc69 Compare October 31, 2017 09:17

derrabus approved these changes Oct 31, 2017

View reviewed changes

ElectricMaxxx force-pushed the prepare_for_symfony_4.0 branch from 5c9dd7b to ca8b1e9 Compare November 3, 2017 05:23

ElectricMaxxx changed the base branch from master to drop-class-parameters November 3, 2017 05:23

ElectricMaxxx force-pushed the prepare_for_symfony_4.0 branch from 3c69900 to 563c982 Compare November 4, 2017 06:45

dbu force-pushed the drop-class-parameters branch from 18a90da to be381b0 Compare November 6, 2017 08:17

dbu closed this Nov 6, 2017

ElectricMaxxx changed the base branch from drop-class-parameters to master November 6, 2017 10:19

ElectricMaxxx reopened this Nov 6, 2017

ElectricMaxxx force-pushed the prepare_for_symfony_4.0 branch from a1f4562 to 5e6ac24 Compare November 6, 2017 10:41

dbu reviewed Nov 6, 2017

View reviewed changes

dbu force-pushed the prepare_for_symfony_4.0 branch from 7a2dd43 to 7a4a234 Compare November 7, 2017 07:58

dbu force-pushed the prepare_for_symfony_4.0 branch from 7a4a234 to bd1267c Compare November 7, 2017 08:33

dbu force-pushed the prepare_for_symfony_4.0 branch from bd1267c to 91887b9 Compare November 7, 2017 12:45

dbu reviewed Nov 7, 2017

View reviewed changes

preparations for symfony version 4.0

795a3e6

after rebase fixes re-add missing symfony version Support Symfony 4.0

ElectricMaxxx force-pushed the prepare_for_symfony_4.0 branch from c061c68 to 795a3e6 Compare November 8, 2017 04:36

ElectricMaxxx merged commit 69848ee into master Nov 8, 2017

ElectricMaxxx deleted the prepare_for_symfony_4.0 branch November 8, 2017 04:53

preparations for symfony 4.0 #286

preparations for symfony 4.0 #286

Conversation

ElectricMaxxx commented Oct 28, 2017 • edited Loading

ElectricMaxxx commented Oct 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbourigault Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Oct 31, 2017

ElectricMaxxx commented Oct 31, 2017

dbu commented Oct 31, 2017

ElectricMaxxx commented Nov 2, 2017

dbu commented Nov 2, 2017

ElectricMaxxx commented Nov 2, 2017

ElectricMaxxx commented Nov 6, 2017

dbu commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElectricMaxxx commented Nov 6, 2017

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElectricMaxxx commented Nov 7, 2017

dbu commented Nov 7, 2017

Choose a reason for hiding this comment

ElectricMaxxx commented Nov 8, 2017

dbu commented Nov 8, 2017

ElectricMaxxx commented Oct 28, 2017 •

edited

Loading

derrabus Oct 30, 2017 •

edited

Loading

fbourigault Oct 30, 2017 •

edited

Loading

derrabus Oct 30, 2017 •

edited

Loading