Pimple3

[henrikbjorn]

This makes Cilex more inline with Silex 2.0 and uses pimple 3. there is an issue with silex/api as have not yet been updated with the pimple 3.0 requirement.

I removed a bunch of providers as they can now be used from silex/providers. Also i moved the ConsoleServiceProvider into Cilex directly, mostly because its a hard requirement. If we want a seperate package for it, we can use a subsplit on src/Cilex/Provider as the silex/providers package does.

[henrikbjorn]

Thix fixes: #49, #48

[henrikbjorn]

Also fixes #43

[henrikbjorn]

Superseeds #47

[mvriel]

Why did you remove the ignore for phpStorm's files?

[henrikbjorn]

should be done from your global .gitignore file

[mvriel]

Hello @henrikbjorn,

First of all, thank you for spearheading this effort to have Cilex 2.0.
What I did notice in this PR is that you have been very enthusiastic about the changes that you have made but also refactored and removed some key elements of Cilex.

It might have been better to split this PR into separate pieces as you are addressing separate issues (I see you removed the Vagrant setup, changed the application class around and more). I also wonder if all code is still properly covered by unit tests because Cilex had a 100% or near 100% test coverage.

Because you made quite invasive decisions it would have been interesting if you could have provided some more information in your PR how you came to these decisions.

[Seldaek]

Anything I can do to help here? Building a new thing with Cilex and it seems this 2.0 would be a lot better with Pimple 3 and require less hand-wiring of providers. @henrikbjorn it'd be great to split up the "opinionated" changes like removing vagrant into a new PR. If it helps I don't mind taking your PR as basis and removing those changes to have a PR with the cilex-related changes only. The rest can be argued later on.

[henrikbjorn]

@Seldaek @mvriel updated with the return of the vagrant setup

[mvriel]

Ow wow.. I did not expect you to make those fixes so fast!

[mvriel]

Issue is now that because there were parts of this PR that I would not want to merge and I wanted to check how it works I have created a new PR based on this one (#57) where I have attributed @henrikbjorn. I think the initiative and work in this PR was far from wasted (I copy-pasted parts of it).

Though I have been a little more conservative by keeping some things, such as the ServiceProviders. Unfortunately Silex cancelled their silex-api and silex/providers package (see the Silex package on packagist; the latest version mentions replacing those packages).

In addition is the ConfigServiceProvider unique to Cilex and in use by phpDocumentor. (as there are a few other things I wanted to keep)

[henrikbjorn]

in Silex the dispatcher is in the Application that is why they HAVE to do the class and then hackish create an instance of it. Here the dispatcher is defined in a ServiceProvider and since we now use EventListenerProviderInterface default subscribers (if we add any) should be added there instead of in the service.

[mvriel]

Ok .. this is a bit awkward. I feel I owe @henrikbjorn apologies for creating the secondary PR..

[henrikbjorn]

The ConfigServiceProvider can be done by other libraries and maybe even better such as github.com/flint/tacker also igor have a ConfigServiceProvider that could be used instead. As this is a new major version i think it would also mean changes to phpdocumentor (if you decided to use it)

[mvriel]

Ok, let me re-check what is preventing me from merging this PR right now

[henrikbjorn]

maybe @Seldaek wants to chime in on the remaining changes

[mvriel]

if we drop the phar installation altogether I can see this working because then there is no real need to have a lock file (if ppl install using composer that will ignore the local lockfile anyway)

[henrikbjorn]

#27 maybe we should change the name of command to add if we want to add the closure thing?

[mvriel]

What still misses here is that the Silex/Api and Silex/Provider packages by Silex are still outdated. I would also prefer to return the ConfigServiceProvider to minimize the BC impact. I agree that the other providers are better served from Silex/Provider since those are exact duplicates.

We should probably make a tree subsplit for our providers; since we already have console, dispatcher and config it would make a good case I think

[henrikbjorn]

I get that loosing the ConfigServiceProvider is a bit of a personal thing, but others to the same or better, sorry :)

I changed a lot based on the comments, and removed the bin/compile since it didnt make sense if we deprecate the phar (which we should)

[mvriel]

I get that loosing the ConfigServiceProvider is a bit of a personal thing, but others to the same or better, sorry :)

Well, it is not as much a personal thing (because I just remembered that phpDocumentor stopped using that ServiceProvider some time ago) but it is a BC break that can have a significant impact on a project. I do not know if and how many projects use the ConfigServiceProvider but exchanging this service provider for a different one that works differently can be a non-trivial task.

On the other hand; I don't think that many will use it and this is a major upgrade; never mind

[mvriel]

Sorry, wrong button

[mvriel]

Thank you for making the changes @henrikbjorn; I truly appreciate this and am going to merge this now. It will sit on develop while test out some changes and give @Seldaek time to check it out before merging to master and tagging 2.0.

I would also prefer it if Silex/Api and Silex/Provider is upgraded so that we can remove the alias off the Pimple entry in composer.json before tagging 2.0

[mvriel]

#27 maybe we should change the name of command to add if we want to add the closure thing?

forgot to reply on this: let's take a look in another PR shall we? It is a good thing to separate features as much as we can in different PRs because it makes communication easier and it does not halt all changes when a single feature in a large PR is subject of discussion

[henrikbjorn]

@mvriel igorw/ConfigServiceProvider#35 makes it possible to use with Cilex 2.0

[Seldaek]

Many thanks to both of you. I now upgraded to the following and it's working great:

    "require": {
        "cilex/cilex": "~2.0@dev",
        "silex/silex": "~2.0@dev",
        "saxulum/saxulum-translation-provider": "~2.0",
        "symfony/twig-bridge": "~2.5",
        // .. project specific stuff below ..
    },

That gives me twig rendering with translation support in cilex commands - I haven't much else to test with yet so I can't give much more feedback so far so good. I'll report back if I find any other issues.

[liuggio]

👍