-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Alternate hotfix for #108 - windows path length limitations #172
Conversation
Ping @malukenho for review :-) |
9e75f2d
to
3c8870c
Compare
$signature = $this->signatureGenerator->generateSignature($parameters); | ||
$defaultProperties = $class->getDefaultProperties(); | ||
|
||
//die(var_dump($defaultProperties, $propertyName, $class->getProperties(), $class->getName())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove debug constructions from the source code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoooops! Thanks for noticing :-)
I want to say, that this PR is much bigger than expected by issue name ) So many classes, aka Java-style, for solving simple root cause in bf7321b. As a developer I like this PR, because it's logical and implemented step-by-step. But as an architect, I want to reject it, because it violates KISS principle by overusing SRP principle for simple tasks (SignatureGenerator, SignatureChecker, ParameterHasher). So, 👍 for PR implementation and 👎 for its complexity |
Fully agree on those thoughts, will think about it further before going on. |
$proxyClassName = $this | ||
->configuration | ||
->getClassNameInflector() | ||
->getProxyClassName($className, $proxyParameters); | ||
|
||
if (! class_exists($proxyClassName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line necessary? Wouldn't the fail-fast check above cover this? If we're at this point the proxy class is yet to be generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line triggers autoloading, which allows usage of an optimized classmap if proxies are generated on deployment to production
…ignatures for generated code
…lways be valid class identifiers
… property in concrete implementations
…nature checker now.
…e class signature generator
3c8870c
to
4e2acd4
Compare
Alternate hotfix for #108 - windows path length limitations
Total issues resolved: **17** - [108: Fix windows path length limitations](#108) - [172: Alternate hotfix for #108 - windows path length limitations](#172) - [178: Verify type-safety with `self` and `static` type-hints](#178) - [180: #178 - `self` type safety check](#180) - [181: Documentation should be released on github pages on the `gh-pages` branch.](#181) - [182: Documentation on github pages](#182) - [187: Proxy generation fails if magic methods are marked as final](#187) - [189: Bugfix - cannot override final methods ( #187 )](#189) - [190: [WIP] [EXPERIMENTAL] Codegen should not trigger fatals](#190) - [191: Put link of documentation on README.md and Close #185](#191) - [193: [WIP] Codegen errors](#193) - [194: Code-generation fatal error prevention](#194) - [195: Blogpost about 1.0.0, 2.0.0 and stability frames](#195) - [196: Define maintainance time-frames (stable/oldstable/etc)](#196) - [198: Highlighting the code examples](#198) - [199: Removed unused `ReflectionMethod` import](#199) - [202: #196 - adding document with expected stability time-frames](#202)
See #108 - the branch name says pretty much everything.
Interesting side-effects though:
ProxyManager
version (forces re-generation of proxies)