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

Remove dependency on Pimple #70

Merged
merged 1 commit into from May 22, 2016

Conversation

aik099
Copy link
Member

@aik099 aik099 commented Nov 11, 2015

Closes #21

@aik099 aik099 force-pushed the 21-remove-dependency-on-pimple branch from 53693fc to b86fb86 Compare November 11, 2015 19:37
@aik099
Copy link
Member Author

aik099 commented Nov 13, 2015

@fabpot is it ok (from licensing point of view) to copy Pimple code under different namespace inside the project (this is what's done in this PR) to avoid conflicts when installed alongside with Silex?

I've kept licensing comment in each Pimple file intact.

@fabpot
Copy link

fabpot commented Nov 14, 2015

From a licensing point of view, I don't have any problems. From a "technical" POV, I don't like this idea all.

@aik099
Copy link
Member Author

aik099 commented Nov 14, 2015

From a "technical" POV, I don't like this idea all.

Any idea on solving? Having dependency on DIC (e.g. Pimple) creates a problems, when another library (e.g. Silex) is using incompatible version of same DIC.

@fabpot
Copy link

fabpot commented Nov 16, 2015

I have another idea. A simple solution would be to have a different package name; that would be enough as the class names are very different between Pimple 1 and Pimple 2/3. So, I propose to create Pimple 4, which would be the same code as Pimple 3 but with a different package name. Then, I would depend on Pimple for Silex, and everyone should then do the same. That way, you could use Pimple 1 and Pimple 4 in the same project.

@aik099
Copy link
Member Author

aik099 commented Nov 16, 2015

Sounds interesting. The Pimple 2.1.1 however does following in Pimple.php file (https://github.com/silexphp/Pimple/blob/v2.1.1/src/Pimple.php):

class_alias('Pimple\Container', 'Pimple');

So if Pimple 1 (from Silex) and Pimple 2.1.1 are used on one project, then it could be tricky, because:

  1. both are from same \ namespace and I have no idea how Composer auto-loader would prioritize them
  2. if Pimple 2.1.1 code is executed first then it creates Pimple class
  3. if Pimple 1 code is executed first, then it creates Pimple class and class_alias call inside Pimple 2 might even result in a notice

@fabpot
Copy link

fabpot commented Nov 16, 2015

I'm only talking about making Pimple 1 and 4 compatible, which should be enough for the vast majority of the projects out there.

@aik099
Copy link
Member Author

aik099 commented Nov 16, 2015

Then every project should migrate to that new pimple-4 package to allow installation together. Of course except Silex 1.

And probably pimple/pimple package needs to be marked as abandoned in favor of new package that would have replace: ... in it's composer.json.

@aik099
Copy link
Member Author

aik099 commented Dec 4, 2015

@fabpot , any update?

2 similar comments
@aik099
Copy link
Member Author

aik099 commented Jan 14, 2016

@fabpot , any update?

@aik099
Copy link
Member Author

aik099 commented Mar 2, 2016

@fabpot , any update?

@fabpot
Copy link

fabpot commented Mar 2, 2016

So, we've tried to make it work and that's just too much work, we gave up. I'm still not very happy with coy/pasting but if that's the only solution...

@aik099
Copy link
Member Author

aik099 commented Mar 2, 2016

So, we've tried to make it work and that's just too much work, we gave up.

You mean you've tried to create a package on Packagist with Pimple 1.0 only and that didn't work?

@adambro
Copy link

adambro commented Mar 9, 2016

Doing copy-paste for such small thing as Pimple is fine IMHO. It reduces coupling and increases cohesion, which is good thing for a library.

Other option would be to depend on container-interop/container-interop package and let user choose DIC. I guess it's more complicated though.

@aik099
Copy link
Member Author

aik099 commented Mar 9, 2016

Other option would be to depend on container-interop/container-interop package and let user choose DIC.

Would this help? Now I'm depending on Pimple 1. After change I will depend on container interop and still depend on Pimple 1 it uses, nope?

@aik099 aik099 force-pushed the 21-remove-dependency-on-pimple branch 2 times, most recently from e3b7d84 to c0230d6 Compare March 10, 2016 08:53
@aik099
Copy link
Member Author

aik099 commented Mar 10, 2016

The failing test on HHVM is due sebastianbergmann/phpunit#2109 bug of PHPUnit. Apparently HHVM doesn't like when DOMDocument class objects are serialized, but normal PHP versions just do it and no error happens for them.

@aik099
Copy link
Member Author

aik099 commented May 12, 2016

@fabpot , any update on this?

@fabpot
Copy link

fabpot commented May 12, 2016

I suppose copy/pasting is the way to go. I just don't have time to investigate other solutions.

@aik099
Copy link
Member Author

aik099 commented May 12, 2016

OK.

@aik099 aik099 merged commit 93a7026 into minkphp:master May 22, 2016
@aik099 aik099 deleted the 21-remove-dependency-on-pimple branch May 22, 2016 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants