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

Improvements #1819

Merged
merged 1 commit into from
Jul 27, 2018
Merged

Improvements #1819

merged 1 commit into from
Jul 27, 2018

Conversation

carusogabriel
Copy link
Contributor

Q A
Type improvement
BC Break no
Fixed issues -

Summary

Together with PHPStorm and https://github.com/kalessil/phpinspectionsea, here are some improvements that we can make to MongoDB ODM, such as unnecessaries parentheses, casts; null coalesce operator usage; in_array instead of array_search + comparison, etc

@@ -750,7 +750,7 @@ private function loadReferenceManyCollectionOwningSide(PersistentCollectionInter
}

// only query for the referenced object if it is not already initialized or the collection is sorted
if (! (($reference instanceof Proxy && ! $reference->__isInitialized__)) && ! $sorted) {
if (! $reference instanceof Proxy && ! $reference->__isInitialized__ && ! $sorted) {
Copy link
Member

Choose a reason for hiding this comment

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

This if-block has a different behaviour than before the changes.

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

@@ -261,7 +261,7 @@ private function addManyReferences(PersistentCollectionInterface $persistentColl

$document = $this->uow->tryGetById($id, $class);

if ($document && ! (($document instanceof Proxy && ! $document->__isInitialized()))) {
if ($document && ! $document instanceof Proxy && ! $document->__isInitialized()) {
Copy link
Member

Choose a reason for hiding this comment

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

This if-block has a different behaviour than before the changes.

@@ -1365,7 +1365,7 @@ private function getClassDiscriminatorValues(ClassMetadata $metadata)
}

// If a defaultDiscriminatorValue is set and it is among the discriminators being queries, add NULL to the list
if ($metadata->defaultDiscriminatorValue && array_search($metadata->defaultDiscriminatorValue, $discriminatorValues) !== false) {
if ($metadata->defaultDiscriminatorValue && in_array($metadata->defaultDiscriminatorValue, $discriminatorValues)) {

This comment was marked as resolved.

This comment was marked as resolved.

@alcaeus alcaeus self-requested a review July 6, 2018 14:29
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

A couple more details, thanks for making these changes!

@@ -12,12 +12,12 @@ class CustomIdType extends Type
{
public function convertToDatabaseValue($value)
{
return $value !== null ? $value : null;
return $value ?? null;
Copy link
Member

Choose a reason for hiding this comment

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

What's the actual point of this line anyways? Couldn't we just make it return $value? No point returning null if the value is already null.

Copy link
Member

Choose a reason for hiding this comment

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

Non-null values were originally cast to a string in ac4ea20#diff-b229b91262ab7a824d0ea63ff9b11c3d; however, that cast was removed in 4645bab#diff-b229b91262ab7a824d0ea63ff9b11c3d.

There's no reason this cannot be changed to simply return the value as-is.

}

public function convertToPHPValue($value)
{
return $value !== null ? $value : null;
return $value ?? null;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member

Choose a reason for hiding this comment

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

Likewise, this can be reduced to return $value;.

unnecessaries parentheses, casts; null coalesce operator usage; in_array
instead of array_search + comparison, etc
@carusogabriel
Copy link
Contributor Author

carusogabriel commented Jul 25, 2018

@alcaeus @jmikola @SenseException Thanks for your feedback. I've amended the commit. Should be RTM.

Sorry for the delay 😅

@carusogabriel carusogabriel reopened this Jul 25, 2018
@alcaeus alcaeus self-assigned this Jul 27, 2018
@alcaeus alcaeus added the Task label Jul 27, 2018
@alcaeus alcaeus added this to the 2.0.0 milestone Jul 27, 2018
@alcaeus alcaeus merged commit ecf0636 into doctrine:master Jul 27, 2018
@alcaeus
Copy link
Member

alcaeus commented Jul 27, 2018

Thanks @carusogabriel!

@carusogabriel carusogabriel deleted the improvements branch July 27, 2018 08:08
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