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

refactored factories for SM v3 #558

Merged
merged 3 commits into from Jul 3, 2016
Merged

refactored factories for SM v3 #558

merged 3 commits into from Jul 3, 2016

Conversation

svycka
Copy link
Contributor

@svycka svycka commented Mar 21, 2016

I couldn't find any plans or work for SM v3 support so decided to at least start something and maybe maintainers will give us some feedback what and how they plan for ZF3.

I tried not to change tests if possible to be sure for BC. But for SM v3 we will need additional tests if everything is ok.

@svycka svycka changed the title refactored factories for sm v3 [WIP] refactored factories for SM v3 Mar 21, 2016
@gianarb
Copy link
Contributor

gianarb commented Mar 22, 2016

Thanks for your effort
it seems good.. I don't see any BC break..
@Ocramius, @TomHAnderson do you have a second to look this PR?

Only to remember: we can merge it when DoctrineORMModule and DoctrineMongoDBODMModule will be migrate.

@TomHAnderson
Copy link
Member

The composer for zend-stdlib must be "^2.5,<=2.6"
https://github.com/doctrine/DoctrineModule/pull/558/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R41
That's not much mileage. So zend-stdlib should be ^2.7 and zend-hydrator ^1.0

If #548 were also put into a release with this it wouldn't mean another such significant BC break in the (near) future.

The PR is clean and although I didn't run unit tests I think it looks fine from an implementation perspective. I'll be glad to develop code that works with these new interfaces.

@svycka
Copy link
Contributor Author

svycka commented Mar 22, 2016

rebased, should be mergeable again.

@TomHAnderson changed zend dependencies to versions that allow php5.5 what do you think now?
also tested with --prefer-lowest tests green at least for php5.6

"zendframework/zend-authentication": "^2.5.2",
"zendframework/zend-cache": "^2.5.2",
"zendframework/zend-paginator": "^2.6",
"zendframework/zend-stdlib": "^2.7.6",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomHAnderson zend-stdlib <2.7.6 versions have BC issues so maybe ^2.7.6 are better? I can change if you want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zend-stdlib > 2.7 https://github.com/zendframework/zend-stdlib/releases/tag/release-2.7.7 no longer has hydrators so this cannot be merged unless zend-stdlib is limited to <= 2.7

I have said in other posts that 2.6 is the limit. I can be sure it's one of these two and 3.0 is completely out of the question so anything > 2.7 is a no-go. So the only long term solution for ^2.7.6 is to merge #548 with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zend-stdlib 2.* does have hydrators, yes main code is moved to zend-hydrator but it still exists and they will not remove them in 2.* releases. And this PR isn't long term solution (just best I can do to support SMv3 without making BC break) you can expect long term solution in DoctrineModule 1.0 version when will be possible to remove stdlib-hydrator. with DoctrineModule 1.0 we can drop support for zend-servicemanager 2.* and zend-stdlib 2.* also can bump zend-hydrator 1.* to 2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I would vote that hydrators should be moved into separate module this could be done now and also would solve a lot problems for other modules depending on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought ^2.7.6 would include 3.0 and above too. At 3.0 the hydrators are no longer in zend-stdlib.

I'm fine with moving the hydrators out. @veewee ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I dont mind if they move. Maybe it's time to rethink the hydrator completely in another repo... If we make it better by default, we don't need the phpro hydrator module anymore.

@TomHAnderson
Copy link
Member

@svycka I'm recommending #548 be merged into this PR. There's more info on that PR here: #546

Upping the versions to your suggestions will break this without #548

I'd like to hear from @gianarb about strategies for moving the ZF Doctrine code forward. My view is, due to @Ocramius remark, that a 2.0-dev branch be started and we merge these two PRs into it at a minimum to start.

@gianarb
Copy link
Contributor

gianarb commented Mar 22, 2016

Hello @TomHAnderson I am working on a blog post about it but yes, the idea it's start a develop branch to work on 2.0..

This PR it's not a BC break because it use stdlib..
Maybe with supports only for ZF3 or something like it.. we are thinking about it

@svycka
Copy link
Contributor Author

svycka commented Mar 22, 2016

@TomHAnderson I can't merge #548 because it contains BC break. Can you be more specific what problems do you see with my suggested versions?

also created doctrine/DoctrineORMModule#459 can someone review?

@TomHAnderson
Copy link
Member

zend-stdlib no longer contins hydrators: 2.7.6 https://github.com/zendframework/zend-stdlib/tree/4499760badfada27ee600f5c2cfd47d5263ccf1b

@svycka
Copy link
Contributor Author

svycka commented Mar 22, 2016

@TomHAnderson what do you mean? do you say that 2.7.6 version is not BC with older versions? but it is.

@TomHAnderson
Copy link
Member

doctrine-module uses the hydrators from zend-stdlib:
https://github.com/TomHAnderson/DoctrineModule/blob/master/src/DoctrineModule/Stdlib/Hydrator/Filter/PropertyName.php#L22

The hydrators no longer exist in zend-stdlib
https://github.com/zendframework/zend-stdlib/tree/master/src

They have moved to zend-hydrator
https://github.com/zendframework/zend-hydrator

This is what #548 fixes

@gianarb
Copy link
Contributor

gianarb commented Mar 22, 2016

They don't longer exist.. But I think that it will exist for all 2.*
versions because remove them it is a BC break
On 22 Mar 2016 10:12 pm, "Tom H Anderson" notifications@github.com wrote:

doctrine-module uses the hydrators from zend-stdlib:

https://github.com/TomHAnderson/DoctrineModule/blob/master/src/DoctrineModule/Stdlib/Hydrator/Filter/PropertyName.php#L22

The hydrators no longer exist in zend-stdlib
https://github.com/zendframework/zend-stdlib/tree/master/src

They have moved to zend-hydrator
https://github.com/zendframework/zend-hydrator

This is what #548 #548
fixes


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#558 (comment)

@svycka
Copy link
Contributor Author

svycka commented Mar 23, 2016

@TomHAnderson hydrators removed only in 3.0 version, from 2.7 they existsas as proxy extending classes from zend-hydrator and keeping BC.

@svycka svycka changed the title [WIP] refactored factories for SM v3 refactored factories for SM v3 Mar 23, 2016
@TomHAnderson
Copy link
Member

@svycka I think you are speaking from what you see and not experience. Keep this problem open in your mind while I build a unit test to prove that zend-hydrator breaks this project.

@svycka
Copy link
Contributor Author

svycka commented Mar 23, 2016

@TomHAnderson that would be great because I already use zend-hydrator and current stable version of doctrinemodule with already allows 2.7 version without any problems so far. But if you know the problem test would be really helpful to understand the problem.

@svycka
Copy link
Contributor Author

svycka commented Mar 24, 2016

Only to remember: we can merge it when DoctrineORMModule and DoctrineMongoDBODMModule will be migrate.

@gianarb why? DoctrineORMModule and DoctrineMongoDBODMModule depends on this and can't be migrated before DoctrineModule.

@gianarb
Copy link
Contributor

gianarb commented Mar 24, 2016

Yes sorry, maybe I wrong to write.. the idea is merge this type of PR for all modules

* @return object
* @throws ServiceNotFoundException if unable to resolve the service.
* @throws ServiceNotCreatedException if an exception is raised when
* creating a service.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just put it on the previous line

@TomHAnderson
Copy link
Member

This describes the problem I'm having with DoctrineModule: phpro/zf-doctrine-hydration-module#20 (comment)

@veewee and I both came to the same conclusions independently.

RSVP

@svycka
Copy link
Contributor Author

svycka commented Apr 5, 2016

@TomHAnderson I understand what you mean, but this PR is not about zend-hydrator. I don't see how zend-hydrator could be added in BC way. We can do this later when will be able to break backwards compatibility.

Also I didn't understood this:

The composer for zend-stdlib must be "^2.5,<=2.6"

@TomHAnderson why we can't have 2.7 version? if we still use stdlib hydrator everything should work. Unless you will give us unit test to understand the problem I think this is ready to merge.

@TomHAnderson
Copy link
Member

OK, I'll put a test back on my todo list. I'll try to push it out by Thursday.

@TomHAnderson
Copy link
Member

@svycka what's the status on this?

@jhonmike
Copy link

👍

@svycka
Copy link
Contributor Author

svycka commented Apr 26, 2016

@TomHAnderson I think this is good to merge, but I am not sure about your problem with hydrator. You say that there is problem with zend-hydrator but this module does not support it so I am not sure this is problem at the moment unless you can provide failing test case. Other than that I think is ready to merge.

@veewee
Copy link

veewee commented Apr 26, 2016

@svycka: The problem @TomHAnderson is describing is that this module isn't supporting zend-hydrator before this PR.
There are some issues with BC between zend-stdlib and zend-hydrator which makes it impossible to use the new zend-hydrator in combination with the doctrine hydrator in this package.

More informations about the problem and failing tests are above: #558 (comment)

@svycka
Copy link
Contributor Author

svycka commented Apr 26, 2016

@veewee yep but this PR is not about zend-hydrator and adding support for it would mean BC break I can't see this change in 0.* I think this can be done only by dropping stdlib hydrator and would be BC break, so only possible in 1.0 version. But this PR is for 0.* so I can't add this here.

@TomHAnderson
Copy link
Member

@svycka @gianarb the only thing I can imagine that you're not understanding about why zend-hydrator needs to be supported is you don't understand my first post on this thread. Here it is again:

The composer for zend-stdlib must be "^2.5,<=2.6" 
https://github.com/doctrine/DoctrineModule/pull/558/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R41
That's not much mileage. So zend-stdlib should be ^2.7 and zend-hydrator ^1.0

If #548 were also put into a release with this it wouldn't mean another such significant BC break in the (near) future.

The PR is clean and although I didn't run unit tests I think it looks fine from an implementation perspective. I'll be glad to develop code that works with these new interfaces.

So if you make this change you must still keep zend-stdlib LESS THAN OR EQUAL TO version 2.6.
What else can I explain that you don't understand in this post?

@svycka
Copy link
Contributor Author

svycka commented Apr 27, 2016

@TomHAnderson sorry but I still don't understand what can I do more without making BC break. Maybe module maintainers can give us some light? what can we do more? Maybe close this PR and go for 1.0 version if this is really that big problem with zend-hydrator?

@zluiten
Copy link
Contributor

zluiten commented Apr 30, 2016

Clearly you guys don't understand each other and neither do I. It's just too bad it's blocking further development.
I've tested this branch with zend-stdlib: ^2.7 every minor version up to 2.7.6 and with zend-hydrator: ^1.0. The tests run fine. Is that what you want @TomHAnderson ?
If there are some BC issues with zend-stdlib < 2.7.6 than they're not affecting this module/pr.

@TomHAnderson
Copy link
Member

TomHAnderson commented May 1, 2016

@svycka Please move forward with this at your own discretion. But what @Ocramius said from the beginning if zend-hydrator requires a BC break please make that now and give us a branch we can work from which uses zend-hydrator. It's been three solid months since my PR for zend-hydrator and if my involvement is causing any sort of delay I'll continue to put my faith in the Doctrine team to keep up with the trends in the Zend community and bow out of this conversation, now.

@svycka
Copy link
Contributor Author

svycka commented May 2, 2016

ping @gianarb

@Slamdunk
Copy link
Contributor

Slamdunk commented Jun 3, 2016

@oprokidnev
Copy link

In my humble opinion there are no reasons to save backward compatibility in migration progress to zf3 components base. Zend service manager/event manger v3 are twice more productive than v2 ones and stable enought for production. It is stupid to save compatibility for better compatibility when we have so big improvements in performance.

@MidnightDesign
Copy link
Contributor

@oprokidnev I agree. Just slap a 2.0.0 on there and we're good, right? Or do I see things too simple?

@aight8
Copy link

aight8 commented Jun 28, 2016

Any update to this PR? ZF3 is released now officially.

@oprokidnev
Copy link

oprokidnev commented Jun 29, 2016

@MidnightDesign it is the simpliest solution. I think it can be even another repo that follows present Zend naming strategy of modules like Doctrine\Zend\Common, Doctrine\Zend\Orm and so on. We can use provide section of composer to save BC for the modules, that requires current modules.

@svycka
Copy link
Contributor Author

svycka commented Jun 29, 2016

@MidnightDesign it is not that easy to release 2.0 if this PR requires just merge and is not merged for 3 months then I don't know when we will have version 2.0 because as I know this requires releasing hydrator and zend-developer-tools into separate modules a bit more refactor for new zf3 improvements and more.

and also a lot third party modules does not work with zf3 yet. So why we can't have zf3 compatibility now and then work for version 2.0 wich can take a lot more time because there is no work done at all. We don't even have a plan for 2.0

@MidnightDesign
Copy link
Contributor

MidnightDesign commented Jun 29, 2016

@svycka Wait, what? The whole ZF3 thing takes forever because we want compatibility with both ZF2 and ZF3, right? Why not just have a 1.x (for ZF2) and a 2.x branch (for ZF3)? I'm sure the 1.x branch would die out pretty quickly. Honestliy, Doctrine is the only thing that keeps me from migrating several projects to ZF3.

And again: if I can help out with something, let me know. I'm more than happy to help.

@manuakasam
Copy link
Contributor

I am completely with this. We do NOT NEED backwards compatibility from DM 2.0 to ZF 2.x - that's bull. That's precisely what mayor versions are there for.

Let's slowly iterate on 2.x for zf3 only. At least that'd be my vote.

@Slamdunk
Copy link
Contributor

👍 for @MidnightDesign approach, no need to have a 2.x with ZF2 BC.

@svycka
Copy link
Contributor Author

svycka commented Jun 29, 2016

@manuakasam this PR is for DM 1.x I agree that 2.x does not need ZF2 compatibility.

I only think that DM 2.0 wont be released soon because there is no code written and plans for it at least I don't know so this will take a lot of time I guess because @Ocramius and @gianarb does not have time at the moment to even merge this. But If someone could create PR for DM 2.0 would be nice :D but until then we have only this PR. So we have two options merge this or wait for DM 2.0 with unknown release date (maybe next year? just a guess, will see). :D

@MidnightDesign
Copy link
Contributor

@svycka You're talking about the MongoDB package, right? I can give that conversion a shot.

@svycka
Copy link
Contributor Author

svycka commented Jun 29, 2016

@MidnightDesign I am talking about DoctrineModule, DoctrienORMModule and DoctrineMongoODMModule all three requires ZF3 support

@michalbundyra
Copy link
Member

Please have a look on #564 - PR prepared for v2 release - support only ZF3.

@gianarb gianarb merged commit d758ed4 into doctrine:master Jul 3, 2016
@gianarb
Copy link
Contributor

gianarb commented Jul 3, 2016

Thanks guys! I am going to release right now!

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