Make convert services #604

ghost · 2020-05-12T16:50:39Z

Branch?	master
Bug fix?	no
New feature?	- yes
Deprecations?	- no
Tickets	- no
License	MIT
Doc PR	- in progress

TODOs:

Maybe bring in more refactoring from @TomasVotruba
Fix/finish checking of Kernel.php for 5.1 recipe code
Clear cache after or recommend the user does
If there is no App\Kernel class, we can't continue. Or, at least, we would need to use a different type-hint in the config files.

weaverryan

This is pretty insane. I love it! Let's see if we can tighten up a few things. But mostly, the tests are critical here, and you've covered them nicely.

src/Maker/MakeConvertServices.php

src/Util/PhpServicesCreator.php

tests/Util/PhpServicesCreatorTest.php

composer.json

src/Maker/MakeConvertPhpServices.php

src/Util/PhpServicesCreator.php

weaverryan

Some comments! Very impressive result! The tweaks are for readability on this complex feature.

Thanks!

src/FileManager.php

src/Maker/MakeConvertPhpServices.php

tests/Util/PhpServicesCreatorTest.php

.../Util/fixtures/YamlConvert/container_built_differently/comparaison_on_container_built_failed

composer.json

src/Maker/MakeConvertPhpServices.php

src/Util/PhpServicesCreator.php

weaverryan · 2020-05-29T14:39:50Z

src/Util/PhpServicesCreator.php

+        if (null === $invalid && ', 0' === $priority) {
+            $priority = null;
+            $innerName = ', null' === $innerName ? null : $innerName;
+        }


I think we are going to have a few more places where we need to intelligently generate methods like this. We could possibly create a private method for it... but I'm not sure if that would be easy. This is just really ugly logic... especially if we repeat it in multiple places.

src/Util/PhpServicesCreator.php

ghost · 2020-06-01T12:39:42Z

I don't understand why my last test is failing.

weaverryan · 2020-06-12T15:52:41Z

I've checked the core YamlFileParser, and we're missing support for these service options:

parent
synthetic
lazy
abstract
file
properties
bind

These should be added to the switch-case statement inside PhpServicesCreator::convertServiceOptionsToNodes. Some will be really easy - I think lazy and synthetic are just true/false values, so they could be added to the same case as shared/public/exclude, etc

Other than that, I'm very happy with this PR!

~~Also, tests should pass once symfony/recipes#785 is merged~~

src/Util/PhpServicesCreator.php

TomasVotruba · 2020-07-09T19:53:29Z

What is missing here?

I'm using this class out in the wild to convert configs in my projects, so I can give some practical feedback

src/Util/PhpServicesCreator.php

TomasVotruba · 2020-07-10T00:21:31Z

src/Util/PhpServicesCreator.php

+        $this->addLineStmt('$parameters = $configurator->parameters();', true);
+
+        foreach ($parameters as $parameterName => $value) {
+            $this->addLineStmt(sprintf(


Actually, all these sprintf(%variable, %call, %args) should not be string, but a node.

Using string in a node as Name, while it is MethodCall or Assing is like writing PHP in one big string.

That's 100% true. I think, at some point (and I think it was quite pragmatic) @archeoprog took some shortcuts, where writing string code was simpler than using the Nodes. In this case (causeI just tried it), it's easy until the argument part, which has a lot of complex logic attached to it - specifically the $this->toString() method.

So I don't think it's worth the work to refactor the internals of the class, though you do have a perfectly valid point.

It makes sense as 1st draft with lack of knowledge of php-parser, I'm 👍 for that.

Just now it would deserve using nodes correctly. As this sprintf magic disables php-parser printer features. Basically printer is now written manually here, which cluters the code and makes it very hard to read and extend. It's too much to think about on every line, instead of extending php-parser printer on 1 place :) (I'm super lazy btw)

E.g. this is how you add indentation to all fluent method calls:

https://github.com/migrify/migrify/pull/45/files#diff-3079c82962d6f72f03af70c687388891R17

Actually, all these sprintf(%variable, %call, %args) should not be string, but a node.

Using string in a node as Name, while it is MethodCall or Assing is like writing PHP in one big string.

That's exactly what I wanted to do with Name, it's a little "hack" reserved for the content of the Closure because the use of nodes proved to be, at this level, a waste of time, very tedious where the manipulation of strings was much simpler. But this is only my personal vision of complexity and now that you have a few sorting algorithms as a basis, feel free to adapt for your project and see what happens with the nodes. I'm sure @weaverryan would be interested to see the result :)

Yeah, it was my first project with the library. I tried a refactoring with nodes. Even without knowing well, the Dump system allows to get nodes corresponding to php instructions so I had this help :)

In spite of that, I couldn't find the interest to continue, surely it would have been necessary to start again from the beginning, impossible for me.

Got you. It makes sense 👍

I look forward to your final version.

weaverryan · 2020-07-10T15:24:37Z

@TomasVotruba!

Ah, super happy to hear you're already using this! The only missing piece is that we really need to convert services_*.yaml all at once - meaning we need to detect (for example) services_test.yaml and put it into the new services.php file. The reason is that, with the 5.1 recipe, once services.yaml is gone, the other services_{env}.yaml files won't be loaded - https://github.com/symfony/recipes/blob/c6de7cf8f54b10a7e4b54729368433fe35fcae0a/symfony/framework-bundle/5.1/src/Kernel.php#L19

So, we need to do that last step - I'm working on it now.

What we really want to know is whether the new services.php file is being read and that it at least has basic functionality. This test now proves that the controller and FakeService are registered correctly and that autowiring works.

…sn't load php files

When we're looping over the service config keys, they are not really "methods", they are service config keys. This clarifies that. Then, set determine the method *based* from that key. This is super minor, but it makes the code read more accurately: we are looping over the config keys and THEN determining the php "method" based on that.

…to the data provider The data provider should, ideally, pass all the info the tests needs directly. So, let's directly tell the test case if the container should be compared or not

…r environments)

TomasVotruba · 2020-07-10T18:58:17Z

Btw, this is the PR with all XML to PHP stuff: https://github.com/migrify/migrify/pull/39
I try to refer it with every change I make, so there is just one source to study.

Use of @migrify tool in practise:

https://github.com/symplify/symplify/pull/1997/files

ghost · 2020-07-10T20:08:11Z

@TomasVotruba

Looks great, i think you should offering an improvment of php parser documentation.
I noticed your big contribution to this library.

In fact, it might be good to present a little bit of the theoretical aspect of code analysis, beyond the library itself. Or maybe, very simply, redirect to documentation, tutorials on this aspect of development.
I didn't learn computer science in school, I realized a lot of theoretical gaps while using it. This is a personal testimony :)

Of course, I imagine your time is precious, it's just a suggestion :)

TomasVotruba · 2020-07-10T20:15:23Z

@archeoprog I'm way to expert for nodes now to be able to write intro documentation :D = only I'd find it useful, nobody else.

I didn't learn computer science in school, I realized a lot of theoretical gaps while using it.

School or knowledge is irelevant here.

I actually haven't studied computer science either, I studied psychology. Docs might be useful, but it would basically 1:1 copy the source. I just looked at the php-parser code and tests and copied the behavior, until I got it right.

The main thing is not documentation, or package code design, but the motivation. If you want to learn AST and nodes just for this single command, it doesn't last long. My motivation is to allow refactoring of any PHP code to modern codebase to every PHP developer in the world without any knowledge at 1 click.

If you feel you'll find a purpose in this, go deep dive to php-parser. It's worth the time and frustration :) soon you'll see PHP code as Matrix code 😉

weaverryan · 2020-07-10T20:26:16Z

@TomasVotruba I've just ported your code over... at least how it looked about 1 hour ago. I know it won't be as clean as your final product, but I'm very happy with it and it's improved several important things that you mentioned. So, thanks again :). I've included links back to the migrify project, of course.

In case it helps, here were a few tweaks I had to make - our test suites clearly have a few edge cases that the other doesn't have:

I tweaked PhpNodeFactory::createImportMethodCall() because the __DIR__ should only be added to the first arg of import(), not all args
In addAliasNode(), the $serviceValues inside the first if could be a string, which is the alias. And if it is, you need to strip off the @. This was causing $services->alias('from', service('to')) - the second argument should not be wrapped in service().
When handling "directory imports" (i.r. if (isset($serviceValues['resource'])) {, the exclude could be a string, not just an array. I also had to add some code so that you could chain other modifiers (e.g. ->tags()) after the load() - check out the services_load_resources_simple.php test case if you're interested - look for ->tags(), they were being left off.
In PhpNodeFactory::createParamValue(), the check for Array_ needs to be recursive (look inside the Array) to find other arrays and make them short. This was causing deep arrays to have things like ['foo', array('bar']] in them. I actually just went one level deeper, instead of recursing.
bind: I think I completely forgot support for this. It's added now, it's simple: it's a subset of arguments.

Cheers!

ghost · 2020-07-10T20:33:34Z

@TomasVotruba

Of course, motivation and go deeper is the best.
But there is already theorical aspects existing for helping us.

For this command, i wrote a lot of things as string. But yes, go deeper is on my todo list but it also depend of my future contributions :)

TomasVotruba · 2020-07-10T22:52:08Z

@weaverryan Very nice job with integration, wow! 👏

I've just added a pile of changes: https://github.com/migrify/migrify/pull/50
I plan to get rid of spacing completelly and let coding standards handle it. It will make the whole deisgn much cleaner and lighter and won't reinvent wheel of php cs fixer/sniffers.

TomasVotruba · 2020-07-10T22:58:58Z

@weaverryan Btw, no need to credit with those annotation. I don't want to spam Symfony, never saw any point in these references of myself. I just connected bunch of ideas that are already out there, nothing special.

Reference in this PR as we discussus is enough.

ghost · 2020-07-10T23:16:26Z

If you have contributed, it's normal. Even if it wasn't credit regarding your components,
We should add your name and mail as an author of the new command.

TomasVotruba · 2020-07-10T23:20:45Z

That's my point. With Github it doesn't make sense anymore. You do git blame and see everything, everyone, everytime. If we should respect the rule, there would be over 50 @author annotations over each file :D

I just say I don't need it here 😉

ghost · 2020-07-11T00:47:05Z

There are few real contributors, giving your opinion or proposing a 10-line PR is usually easy. You didn't just give your opinion. Ryan's in charge of completing Command and the Merge. We'll see what he thinks about it :)

TomasVotruba · 2020-07-11T15:56:33Z

@weaverryan New pull-request since yesterday evening:

I'm in 2/3 of my goal

weaverryan · 2020-07-11T17:36:04Z

Ha! You're on fire @TomasVotruba :)

I don't know if I'll bring over another round of changes or not. My guess based on migrify/migrify#51 is that you will have crushed the bugs that I had to fix when porting over, which would make the job much easier. The last significant change that I can think of is the cs. You mentioned:

I plan to get rid of spacing completelly and let coding standards handle it. It will make the whole deisgn much cleaner and lighter and won't reinvent wheel of php cs fixer/sniffers.

That makes sense to me too. I'll be interested to see how different your final CS is from our's and what changes would be needed to make them match again. Some things are 100% subjective, like whether or not we should have line breaks between $parameter->set() calls (e.g. https://github.com/migrify/migrify/pull/54/files#diff-96075827718f599e136fc063d29dda47)

Cheers!

TomasVotruba · 2020-07-11T18:30:59Z

This should be the last one to get rid of manually formating:
https://github.com/migrify/migrify/pull/54

I preserved most of the coding standards, except arrays (there are sniffs/fixer that handle it better and keep the formatting even after manual change).

I have added the breaks between set parameters back, now that's easy to handle it in the printer.

The main change was where the coding standard is applied. I do not handle it in transformation itself, but nicely sperated in the printer. It's like having a controller and template separated :).

TomasVotruba · 2020-07-11T18:36:01Z

Now I only plan to decouple the huge god class switch to standalone smaller classes with collector pattern. Just to tidy up the mess and lower the code complexity.

Class with 900+ lines is pain to work with. The split collectors will handle first-level keys.

The functionally remains the same. I think it's done (till first bug report). I plan to use it on my packages next week.

TomasVotruba · 2020-07-12T11:25:28Z

@weaverryan FYI, I'm done for now with package. It's ready to be ported 👍
Let me know if you need to explain some parts.

ghost · 2020-07-13T18:14:47Z

I see that you haven't totally removed the use of Name as a "hack" to write lines.

In fact, I figured if I can't convert everything to Nodes, I'll keep this trick for most methods. In order not to disturb the eventual readers of the code.

But the new code is really beautiful, i love it!

TomasVotruba · 2020-07-13T19:14:09Z

@archeoprog Not sure who you talk to without the @ handle :D but in case it was me...

I see that you haven't totally removed the use of Name as a "hack" to write lines.

I have actually. It should not be there at all... where is it?

(If it's for @weaverryan , ignore this message).

TomasVotruba · 2020-07-14T11:32:00Z

This how @migrify tool runs on @symplify configs:

https://github.com/symplify/symplify/pull/2005/files

@weaverryan I found a new problem that might be useful to you to know. The service() was added only in Symfony 5.1. Before the same function was called ref(). Just for better compatibility I use ref()

ghost · 2020-07-17T15:44:57Z

It was for Ryan :)

I found a new problem that might be useful to you to know. The service() was added only in Symfony 5.1. Before the same function was called ref(). Just for better compatibility I use ref()

That was my original intention but it seems that Ryan and the Symfony team prefers this feature to be available from
Symfony 5.1 only.

TomasVotruba · 2020-08-06T23:03:05Z

@archeoprog Ping, I think your account might have been hacked.

ghost · 2020-08-06T23:12:25Z

No, don't worry. I just want to impose my opinion as most of libraries have been doing for several weeks.

ghost · 2020-11-30T01:29:27Z

Giving up so much code for ideological reasons @weaverryan

ghost marked this pull request as draft May 12, 2020 16:50

weaverryan reviewed May 12, 2020

View reviewed changes

weaverryan reviewed May 14, 2020

View reviewed changes

composer.json Outdated Show resolved Hide resolved

weaverryan reviewed May 15, 2020

View reviewed changes

weaverryan reviewed May 23, 2020

View reviewed changes

weaverryan reviewed May 29, 2020

View reviewed changes

ghost commented Jun 1, 2020

View reviewed changes

src/Util/PhpServicesCreator.php Show resolved Hide resolved

weaverryan mentioned this pull request Jun 12, 2020

Allowing php config in the kernel symfony/recipes#785

Merged

ghost commented Jun 12, 2020

View reviewed changes

src/Util/PhpServicesCreator.php Show resolved Hide resolved

yceruto mentioned this pull request Jun 12, 2020

Move internal Container configuration from XML to PHP symfony/symfony#37186

Closed

40 tasks

TomasVotruba mentioned this pull request Jul 9, 2020

[ConfigTransformer] Add Xml To PHP deprecated-packages/migrify#39

Merged

5 tasks

TomasVotruba reviewed Jul 10, 2020

View reviewed changes

src/Util/PhpServicesCreator.php Outdated Show resolved Hide resolved

TomasVotruba reviewed Jul 10, 2020

View reviewed changes

jean and others added 13 commits July 10, 2020 11:31

Adding command to convert service yaml files to php

1f3022f

adding an argument to control the filename

4f77ceb

helping edit know specifically which class is returned

668cd3f

re-adding Symfony 5.1 requirement

a02f9f2

improving the test

b7201c5

What we really want to know is whether the new services.php file is being read and that it at least has basic functionality. This test now proves that the controller and FakeService are registered correctly and that autowiring works.

Adding bigger closing statement, including warning if your kernel doe…

9e1c4aa

…sn't load php files

fixing some misspellings

52b4191

adding test case for new deprecated syntax in 5.

027f969

convert exceptions from conversion into the nicer command exceptions

6853a09

moving the handling of whether or not the container should be tested …

f791e72

…to the data provider The data provider should, ideally, pass all the info the tests needs directly. So, let's directly tell the test case if the container should be compared or not

this file is already tested in the main method

11198df

renaming directory

579b780

adding support for converting all services files at the same time (fo…

60aadd5

…r environments)

avoiding state problem and fixing "main" -> "all"

d8f12c7

Bringing in changes from https://github.com/migrify/migrify

098295e

TomasVotruba mentioned this pull request Jul 11, 2020

[ConfigTransformer] Add maker-bundle test fixtures deprecated-packages/migrify#51

Merged

fixing expected variable

5033a64

weaverryan closed this Nov 9, 2020

symfony deleted a comment Nov 9, 2020

nicolas-grekas mentioned this pull request Jan 18, 2021

Cleanup Kernel & services.yaml files symfony/recipes#877

Closed

Make convert services #604

Make convert services #604

Conversation

ghost commented May 12, 2020 • edited by ghost Loading

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan May 29, 2020

Choose a reason for hiding this comment

ghost commented Jun 1, 2020 • edited by ghost Loading

weaverryan commented Jun 12, 2020 • edited Loading

TomasVotruba commented Jul 9, 2020

TomasVotruba Jul 10, 2020

Choose a reason for hiding this comment

weaverryan Jul 10, 2020

Choose a reason for hiding this comment

TomasVotruba Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

ghost Jul 10, 2020

Choose a reason for hiding this comment

ghost Jul 10, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

TomasVotruba Jul 10, 2020

Choose a reason for hiding this comment

weaverryan commented Jul 10, 2020

TomasVotruba commented Jul 10, 2020 • edited Loading

ghost commented Jul 10, 2020

TomasVotruba commented Jul 10, 2020 • edited Loading

weaverryan commented Jul 10, 2020

ghost commented Jul 10, 2020

TomasVotruba commented Jul 10, 2020 • edited Loading

TomasVotruba commented Jul 10, 2020

ghost commented Jul 10, 2020

TomasVotruba commented Jul 10, 2020

ghost commented Jul 11, 2020

TomasVotruba commented Jul 11, 2020 • edited Loading

weaverryan commented Jul 11, 2020

TomasVotruba commented Jul 11, 2020

TomasVotruba commented Jul 11, 2020

TomasVotruba commented Jul 12, 2020 • edited Loading

ghost commented Jul 13, 2020 • edited by ghost Loading

TomasVotruba commented Jul 13, 2020 • edited Loading

TomasVotruba commented Jul 14, 2020

ghost commented Jul 17, 2020 • edited by ghost Loading

TomasVotruba commented Aug 6, 2020

ghost commented Aug 6, 2020 • edited by ghost Loading

ghost commented Nov 30, 2020

ghost commented May 12, 2020 •

edited by ghost

Loading

ghost commented Jun 1, 2020 •

edited by ghost

Loading

weaverryan commented Jun 12, 2020 •

edited

Loading

TomasVotruba Jul 10, 2020 •

edited

Loading

ghost Jul 10, 2020 •

edited by ghost

Loading

TomasVotruba commented Jul 10, 2020 •

edited

Loading

TomasVotruba commented Jul 10, 2020 •

edited

Loading

TomasVotruba commented Jul 10, 2020 •

edited

Loading

TomasVotruba commented Jul 11, 2020 •

edited

Loading

TomasVotruba commented Jul 12, 2020 •

edited

Loading

ghost commented Jul 13, 2020 •

edited by ghost

Loading

TomasVotruba commented Jul 13, 2020 •

edited

Loading

ghost commented Jul 17, 2020 •

edited by ghost

Loading

ghost commented Aug 6, 2020 •

edited by ghost

Loading