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

Drop doctrine/common proxies in favor of ProxyManager #1875

Merged
merged 1 commit into from
Nov 12, 2018

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Oct 9, 2018

Q A
Type feature
BC Break yes
Fixed issues #1813

Summary

This PR replaces the proxy implementation from doctrine/common with ocramius/proxy-manager. Tests are stable, with the exception of two tests that were no longer relevant. BC breaks still need to be documented.

As for a possible BC layer, I'm not sure what this would look like or if we want to spend the time creating one.

Todos:

  • Prepare BC layer for 1.3? (separate PR)
  • Document BC breaks in UPGRADE document
  • Update documentation
  • Replace calls to get_class with calls to ClassNameResolver, in which case it should be moved out of the Proxy namespace?

Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

Indeed the changes are smaller than one could anticipate :)

lib/Doctrine/ODM/MongoDB/Configuration.php Outdated Show resolved Hide resolved
lib/Doctrine/ODM/MongoDB/Configuration.php Outdated Show resolved Hide resolved
lib/Doctrine/ODM/MongoDB/Configuration.php Outdated Show resolved Hide resolved
lib/Doctrine/ODM/MongoDB/Configuration.php Outdated Show resolved Hide resolved
lib/Doctrine/ODM/MongoDB/Proxy/ClassNameResolver.php Outdated Show resolved Hide resolved
lib/Doctrine/ODM/MongoDB/Configuration.php Show resolved Hide resolved
@alcaeus
Copy link
Member Author

alcaeus commented Oct 19, 2018

Regarding generation strategies, I'd like to give those some though. Here's what we used to have:

  • AUTOGENERATE_ALWAYS: Regardless of whether cached proxy file exists, regenerate and write to disk
  • AUTOGENERATE_FILE_NOT_EXISTS: If cached proxy file does not exists, generate and write to disk for future use
  • AUTOGENERATE_EVAL: Always use eval to create proxies
  • AUTOGENERATE_NEVER: Throw an error if requested proxy was not found in cache

Proxy Manager now only has two strategies that we can currently use:

  • FileWriterGeneratorStrategy: writes cached files to disk, essentially replicating AUTOGENERATE_FILE_NOT_EXISTS
  • EvaluatingGeneratorStrategy: uses eval to generate proxy classes

We could write our own strategies to replicate the always and never options. always is quite useful for development because you don't need to worry about clearing the cache while never is nice for production to prevent performance regressions. However, I'm not sure if we want to keep them around, WDYT?

@malarzm
Copy link
Member

malarzm commented Oct 19, 2018

  • AUTOGENERATE_ALWAYS - IMO this could be removed, it's good for dev usage where _EVAL will do just fine as well.
  • AUTOGENERATE_NEVER - personally I've never used this, IIRC this could be useful for read-only filesystems to ensure Doctrine won't try to write to disk, I think this strategy should stay.

@stof
Copy link
Member

stof commented Oct 19, 2018

AUTOGENERATE_NEVER was added to match the old behavior of doctrine/common when adding more strategies, to allow preserving BC. But FILE_NOT_EXISTS is a better solution anyway.

In case of read-only filesystem, both strategies have the same shortcoming: they both require you to generate all your proxies upfront:

  • never always have this requirement (it will require the file without checking for existence, so a non-existent file is a Fatal Error)
  • file_not_exists will try to create the missing file, and will fail on read-only filesystems.

So I don't thing the never strategy is actually necessary to support read-only filesystems. If you can generate all proxies upfront, file_not_exists will work just fine for you. and if you cannot, your only choice will be to use eval anyway.

@alcaeus
Copy link
Member Author

alcaeus commented Oct 29, 2018

Ok, so we're deprecating AUTOGENERATE_NEVER in favour of AUTOGENERATE_FILE_EXISTS and AUTOGENERATE_ALWAYS in favour of AUTOGENERATE_EVAL in 1.3 and dropping them in 2.0. I'll keep the constants around for future compatibility 👍

@alcaeus alcaeus force-pushed the replace-proxy-implementation branch from 6cc89d6 to 1d94693 Compare October 29, 2018 06:55
Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

Big 👍 beside doc changes

UPGRADE-2.0.md Outdated Show resolved Hide resolved
UPGRADE-2.0.md Outdated Show resolved Hide resolved
@malarzm
Copy link
Member

malarzm commented Oct 29, 2018

About that "big 👍" - tests are quite failing 🙈

@alcaeus
Copy link
Member Author

alcaeus commented Oct 29, 2018

Weird, they were quite stable on my machine. Will have to check that out later.

@alcaeus alcaeus force-pushed the replace-proxy-implementation branch 2 times, most recently from 9fea083 to 3d019ac Compare October 30, 2018 06:17
@alcaeus alcaeus force-pushed the replace-proxy-implementation branch from 3d019ac to 4681d8b Compare November 9, 2018 06:39
@alcaeus alcaeus merged commit b8f7dd4 into doctrine:master Nov 12, 2018
@alcaeus alcaeus deleted the replace-proxy-implementation branch November 12, 2018 06:45
@Majkl578
Copy link
Contributor

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants