Remove rest dependencies #173

loic425 · 2020-07-05T12:56:30Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	fixes #170
License	MIT

lchrusciel

Seems very much ok, but I would prefer to have someone else to check it as well

src/Bundle/DependencyInjection/Compiler/RegisterViewHandlerPass.php

loic425 · 2020-07-09T18:44:54Z

I think we have to add these librairies on conflict to prevent installing older versions than compatible ones. See Symfony composer requirements...

loic425 · 2020-07-09T19:20:04Z

I'd like to test prod requirements too. but I don't manage to launch clear cache command on --env=prod after composer install --no-dev. Test app still have test env...

loic425 · 2020-07-13T09:57:19Z

@pamil @lchrusciel I need help to test prod requirements. It doesn't work with my last commit.
When I run this command "(cd src/Bundle/test && app/console cache:clear --env=prod)" I am on test environment.

lchrusciel · 2020-07-14T13:53:28Z

Hey Loic, sorry for not being to responsive. We had a few commitments that we had to fulfill recently. However, with 1.8 release coming, you can expect more power to review and push things further.

In the same time, thanks a lot for your patience and work :)

loic425 · 2020-09-02T13:37:14Z

@pamil @lchrusciel I rebased this PR to fix conflicts. I can squash all commits into one if you want.

lchrusciel · 2020-09-08T07:27:49Z

Hey Loic,

thanks for your contribution and again sorry for the long feedback loop. However, we have to fix the main build before we can go anywhere further.

loic425 · 2020-09-09T15:23:01Z

@lchrusciel I fixed the Ci build.

loic425 · 2020-09-25T06:38:44Z

@lchrusciel @pamil This one is ready and build is fixed. Is there anything else to improve?

loic425 · 2020-09-29T09:10:53Z

@lchrusciel I made a little change to use on-invalid="null", it allows to remove a compiler pass just for that case.

src/Bundle/DependencyInjection/Driver/AbstractDriver.php

src/Bundle/Resources/config/services/controller.xml

pamil · 2020-10-12T12:35:23Z

src/Bundle/test/app/AppKernel.php

+        if ('prod' === $this->getEnvironment()) {
+            $loader = require __DIR__ . '/../../../../vendor/autoload.php';
+            $loader->addPsr4('AppBundle\\', __DIR__ . '/../src/AppBundle/');
+            $loader->addPsr4('Webmozart\\Assert\\', __DIR__ . '/../../../Component/vendor/webmozart/assert/src/');


I guess it's related to #188 and it should be fixed.

@pamil Maybe, we could fix #188 and rebase this one (again).

Done in #189 🎉

@pamil I rebased (again)

pamil · 2020-10-13T09:43:28Z

composer.json

        "winzou/state-machine-bundle": "^0.3.2 || ^0.4.3 || ^0.5"
    },
    "replace": {
        "sylius/resource": "self.version"
    },
    "conflict": {
-        "amphp/amp": "^2.4.3"
+        "amphp/amp": "^2.4.3",
+        "friendsofsymfony/rest-bundle": "<3.0",


I wonder whether we shouldn't also add conflict for the next major version (to be removed when we decide to support it). Then it would be <3.0 >=4.0 (and similar for other requirements here). What do you think?

src/Bundle/DependencyInjection/Driver/AbstractDriver.php

pamil · 2020-10-13T09:46:33Z

src/Bundle/test/app/AppKernel.php

-        return [
+        if ('prod' === $this->getEnvironment()) {
+            $loader = require __DIR__ . '/../../../../vendor/autoload.php';
+            $loader->addPsr4('AppBundle\\', __DIR__ . '/../src/AppBundle/');


Is it needed? autoload-dev in composer.json should already do the job.

Nope, it's removed on "prod" env.

When do we run the test application in prod environment?

src/Bundle/test/app/AppKernel.php

pamil · 2020-10-13T09:47:23Z

src/Bundle/test/app/console


-$application = new Application(new AppKernel('test', true));
+$application = new Application(new AppKernel($env, true));


What's the idea behind that? Do we really need it?

The idea is to test prod requirements (without optional rest bundles).

@pamil Maybe you have a better idea to test without optional bundles...

Co-authored-by: Kamil Kokot <kamil@kokot.me>

loic425 · 2020-10-13T12:07:49Z

.travis.yml

+
+    - composer require doctrine/orm:^2.5 --no-update --no-scripts --prefer-dist
+    - composer update --no-dev --prefer-dist
+    - (cd src/Bundle/test && app/console cache:clear --env=prod)


@pamil we test prod environment here.

pamil · 2020-10-13T14:14:56Z

src/Bundle/test/app/AppKernel.php

-
-        return parent::getContainerBaseClass();
+        $loader->load(__DIR__ . '/config/{config}.{php,xml,yaml,yml}', 'glob');
+        $loader->load(__DIR__ . '/config/{config}_' . $this->getEnvironment() . '.{php,xml,yaml,yml}', 'glob');


Makes the environment config file optional, it's the same behaviour as in Symfony 4+ apps.

pamil · 2020-10-13T14:15:09Z

src/Bundle/test/app/console

@@ -19,7 +19,7 @@ use Symfony\Bundle\FrameworkBundle\Console\Application;
 use Symfony\Component\Console\Input\ArgvInput;

 $input = new ArgvInput();
-$env = $input->getParameterOption(array('--env', '-e'), getenv('SYMFONY_ENV') ?: 'test');
+$env = $input->getParameterOption(['--env', '-e'], getenv('APP_ENV') ?? 'test');


Replaced SYMFONY_ENV to more recent APP_ENV.

pamil · 2020-10-13T15:10:41Z

Thank you, Loïc! 🥇

loic425 · 2020-10-13T17:09:01Z

Thank you, Loïc! 🥇

Thank you too @pamil. You worked a lot on this one too.

loic425 requested a review from a team as a code owner July 5, 2020 12:56

loic425 force-pushed the feature/remove-rest-dependencies branch 7 times, most recently from 20dbf31 to 4919a03 Compare July 6, 2020 08:49

loic425 changed the title ~~[WIP] Remove rest dependencies~~ Remove rest dependencies Jul 6, 2020

lchrusciel approved these changes Jul 9, 2020

View reviewed changes

src/Bundle/DependencyInjection/Compiler/RegisterViewHandlerPass.php Outdated Show resolved Hide resolved

loic425 force-pushed the feature/remove-rest-dependencies branch 6 times, most recently from 16efd54 to e45c941 Compare July 13, 2020 15:50

loic425 force-pushed the feature/remove-rest-dependencies branch 6 times, most recently from 039cbd6 to 6458a38 Compare July 15, 2020 09:11

loic425 requested a review from lchrusciel July 15, 2020 09:14

stloyd approved these changes Jul 31, 2020

View reviewed changes

loic425 force-pushed the feature/remove-rest-dependencies branch from 6458a38 to 0467038 Compare September 2, 2020 13:32

loic425 force-pushed the feature/remove-rest-dependencies branch 2 times, most recently from dbe6b0b to b26bba8 Compare September 9, 2020 07:32

lchrusciel linked an issue Sep 9, 2020 that may be closed by this pull request

Symfony 5 support #121

Closed

loic425 force-pushed the feature/remove-rest-dependencies branch from b26bba8 to 3cb71cc Compare September 9, 2020 13:54

loic425 force-pushed the feature/remove-rest-dependencies branch from bc44a0d to 606e4b2 Compare September 29, 2020 09:03

pamil reviewed Oct 12, 2020

View reviewed changes

loic425 force-pushed the feature/remove-rest-dependencies branch 3 times, most recently from 742819e to 1f86800 Compare October 12, 2020 16:04

Remove rest dependencies

6a359ff

loic425 force-pushed the feature/remove-rest-dependencies branch from 1f86800 to 6a359ff Compare October 12, 2020 19:21

Fix abstract driver

d4b20a8

loic425 mentioned this pull request Oct 13, 2020

Remove conflict with amphp/amp #192

Merged

pamil reviewed Oct 13, 2020

View reviewed changes

loic425 and others added 2 commits October 13, 2020 12:00

Update src/Bundle/DependencyInjection/Driver/AbstractDriver.php

09fc0fa

Co-authored-by: Kamil Kokot <kamil@kokot.me>

Update src/Bundle/test/app/AppKernel.php

3dfe0c3

Co-authored-by: Kamil Kokot <kamil@kokot.me>

loic425 commented Oct 13, 2020

View reviewed changes

pamil added 2 commits October 13, 2020 16:12

Replace "prod" environment with "test_without_fosrest" one

24fe9f2

Add conflict for next major releases of optional dependencies

c197044

pamil force-pushed the feature/remove-rest-dependencies branch from 3298d3e to c197044 Compare October 13, 2020 14:13

pamil reviewed Oct 13, 2020

View reviewed changes

Different way to remove FOSRestBundle with dependencies

69f3596

pamil merged commit 3b19604 into Sylius:master Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove rest dependencies #173

Remove rest dependencies #173

loic425 commented Jul 5, 2020

lchrusciel left a comment

loic425 commented Jul 9, 2020

loic425 commented Jul 9, 2020

loic425 commented Jul 13, 2020 •

edited

Loading

lchrusciel commented Jul 14, 2020

loic425 commented Sep 2, 2020

lchrusciel commented Sep 8, 2020

loic425 commented Sep 9, 2020

loic425 commented Sep 25, 2020

loic425 commented Sep 29, 2020

pamil Oct 12, 2020

loic425 Oct 12, 2020

pamil Oct 12, 2020

loic425 Oct 12, 2020

pamil Oct 13, 2020

pamil Oct 13, 2020

loic425 Oct 13, 2020

pamil Oct 13, 2020

pamil Oct 13, 2020

loic425 Oct 13, 2020

loic425 Oct 13, 2020

loic425 Oct 13, 2020

pamil Oct 13, 2020

pamil Oct 13, 2020

pamil commented Oct 13, 2020

loic425 commented Oct 13, 2020 •

edited

Loading


		$application = new Application(new AppKernel('test', true));
		$application = new Application(new AppKernel($env, true));

Remove rest dependencies #173

Remove rest dependencies #173

Conversation

loic425 commented Jul 5, 2020

lchrusciel left a comment

Choose a reason for hiding this comment

loic425 commented Jul 9, 2020

loic425 commented Jul 9, 2020

loic425 commented Jul 13, 2020 • edited Loading

lchrusciel commented Jul 14, 2020

loic425 commented Sep 2, 2020

lchrusciel commented Sep 8, 2020

loic425 commented Sep 9, 2020

loic425 commented Sep 25, 2020

loic425 commented Sep 29, 2020

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

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

pamil commented Oct 13, 2020

loic425 commented Oct 13, 2020 • edited Loading

loic425 commented Jul 13, 2020 •

edited

Loading

loic425 commented Oct 13, 2020 •

edited

Loading