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

EZP-26885: As a Developer I want to future proof my Field Types by using Doctrine [in external storage] #1993

Merged
merged 11 commits into from Jul 3, 2017

Conversation

3 participants
@alongosz
Copy link
Member

alongosz commented May 22, 2017

Part 2 of EZP-26885 implementation (part 1: #1985).

We want to be able to use \Doctrine\DBAL\Connection for FieldTypes external storage directly.
To achieve this, FieldType external storage needs to be injected with DoctrineStorage gateway which uses Doctrine instead of Legacy Connection Handler.

This PR, using changes from #1985, introduces FT ext. storage-specific DoctrineStorage gateway which gets injected with \Doctrine\DBAL\Connection.

TODO:

  • Remove TMP commit containing #1985 and rebase after it gets merged.
  • Create DoctrineStorage gateway for Keyword FT external storage (c1b2a9c).
  • Create DoctrineStorage gateway for Url FT external storage (0f8d848).
  • Create DoctrineStorage gateway for RichText FT external storage (83cad0f).
  • Create DoctrineStorage gateway for BinaryFile and Media FT external storage (15ef9f5).
  • Create DoctrineStorage gateway for Image FT external storage (e6031a4).
  • Create DoctrineStorage gateway for MapLocation FT external storage (6acc88f).
  • Create DoctrineStorage gateway for User FT external storage (b0eb5a8).
  • Create DoctrineStorage gateway for Page FT external storage (656b9ed).
  • Add deprecation doc and warnings to the LegacyStorage gateways (84c8a7e).
  • Perform manual tests with web app.

@alongosz alongosz force-pushed the alongosz:ezp-26885-ft-ext-storage-doctrine branch 3 times, most recently from b6f3be5 to e70f5d0 May 22, 2017

@alongosz alongosz changed the title [WIP] EZP-26885: As a Developer I want to future proof my Field Types by using Doctrine [in external storage] EZP-26885: As a Developer I want to future proof my Field Types by using Doctrine [in external storage] May 26, 2017

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented May 29, 2017

Note for reviewers: for now this PR includes also #1985 so it's best to review commit by commit (links in the description above).

@alongosz alongosz requested review from andrerom and Nattfarinn May 29, 2017

@kore

This comment has been minimized.

Copy link
Contributor

kore commented Jun 19, 2017

+1

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jun 19, 2017

Looks good, but will do full review once #1985 has been merged and this has been rebased.

Or even better would be if you can set base branch to #1985 to be able to rebase on top of it already.

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jun 20, 2017

Or even better would be if you can set base branch to #1985 to be able to rebase on top of it already.

That would be ideal, but #1985 branch is on my fork, and I cannot edit base fork for PR (looks like it's possible to choose fork only when creating PR).
However I could also close this one and create another one pointing to #1985 base branch.

@alongosz alongosz force-pushed the alongosz:ezp-26885-ft-ext-storage-doctrine branch from e70f5d0 to 2dd6c46 Jun 22, 2017

@alongosz alongosz force-pushed the alongosz:ezp-26885-ft-ext-storage-doctrine branch from 2dd6c46 to 15ef9f5 Jun 30, 2017

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jul 3, 2017

Looks good. But I'm missing something fundamentally here I think, shouldn't the legacy once be deprecated?

Also how do we handle the possible schema differences between legacy and improved engine once we introduce that?

@andrerom
Copy link
Member

andrerom left a comment

On further thinking I think we accept this, but with deprecation of the old legacy gateways which are now not used anymore.

For the schema discussion we can tackle that once we get there, we don't have enough info yet on what we need and how to avoid people having to migrate code when schema is the same vs different and so on between engines.

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jul 3, 2017

On further thinking I think we accept this as is, but with deprecation of the old legacy gateways which are now not used anymore.

Ok, makes sense 👍

@alongosz

This comment has been minimized.

Copy link
Member Author

alongosz commented Jul 3, 2017

with deprecation of the old legacy gateways which are now not used anymore.

84c8a7e: Added PhpDoc deprecation tag and triggered E_USER_DEPRECATED in constructors.
(Please note that BinaryFileStorage and MediaStorage use common constructor in BinaryBaseStorage).

@andrerom andrerom merged commit d114308 into ezsystems:master Jul 3, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
ezrobot/phpcsfixer Code review by ezrobot
Details

@alongosz alongosz deleted the alongosz:ezp-26885-ft-ext-storage-doctrine branch Jul 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.