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

Convert proxy factory auto generate mode to integer #816

Merged
merged 1 commit into from Aug 31, 2017

Conversation

dragosprotung
Copy link
Contributor

Clean PR based on #815

Copy link
Contributor

@Majkl578 Majkl578 left a comment

Choose a reason for hiding this comment

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

LGTM, just two trivial things. :)

self::AUTOGENERATE_NEVER,
self::AUTOGENERATE_ALWAYS,
self::AUTOGENERATE_FILE_NOT_EXISTS,
self::AUTOGENERATE_EVAL
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be comma after last item.

*/
public function __construct(ProxyGenerator $proxyGenerator, ClassMetadataFactory $metadataFactory, $autoGenerate)
{
$this->proxyGenerator = $proxyGenerator;
$this->metadataFactory = $metadataFactory;
$this->autoGenerate = (bool)$autoGenerate;

if ( ! in_array($autoGenerate, self::AUTOGENERATE_MODES, false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need false here as it's default behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the default value, but I wanted to be explicit that we allow other values

Copy link
Member

Choose a reason for hiding this comment

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

And why not casting it to int and making a strict comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I don't set properties until they are validated. But I guess in this case makes no difference as it's the constructor

@lcobucci
Copy link
Member

@dragosprotung there are some CS violations, I've just pushed #817 to update our build process and verify coding standard violations. Do you want me to point out the issues or do you prefer to wait for it to be merged and just use phpcs/phpcbf?

@dragosprotung
Copy link
Contributor Author

@lcobucci I can see them from your PR, but I will wait for it to be merged if it will not take too long

@Ocramius
Copy link
Member

@lcobucci let's not pick on CS if a massive conflicting PR is coming in anyway :-P

@@ -69,6 +69,13 @@
*/
const AUTOGENERATE_EVAL = 3;

private const AUTOGENERATE_MODES = [
Copy link
Member

Choose a reason for hiding this comment

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

YAY!

@Ocramius Ocramius self-assigned this Aug 31, 2017
@Ocramius Ocramius added the Bug label Aug 31, 2017
@Ocramius Ocramius added this to the 2.8.1 milestone Aug 31, 2017
Ocramius added a commit that referenced this pull request Aug 31, 2017
…oxy-generator-modes-2.8' into 2.8

Backport #815
Backport #816
@Ocramius Ocramius merged commit b000432 into doctrine:master Aug 31, 2017
Ocramius added a commit that referenced this pull request Aug 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants