Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix type guesser. Ignore field MappingException. #458

Merged
merged 1 commit into from
Apr 19, 2018
Merged

Fix type guesser. Ignore field MappingException. #458

merged 1 commit into from
Apr 19, 2018

Conversation

BoShurik
Copy link
Contributor

Fix errors like "No mapping found for field 'plainPassword' in class 'App\Document\Administrator'."

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.

@BoShurik thanks a lot for the PR! Could you please add a test to prevent regression in the future?

@@ -49,6 +49,10 @@ public function guessType($class, $property)

list($metadata, $name) = $ret;

if (!$metadata->hasField($property)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a space after !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@malarzm Done. But in code there are only 3 places with such style. And 22 without spaces

Copy link
Member

Choose a reason for hiding this comment

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

Ah so the bundle did not receive CS fixes yet, thank you :)

@@ -49,6 +49,10 @@ public function guessType($class, $property)

list($metadata, $name) = $ret;

if (! $metadata->hasField($property)) {
return new TypeGuess($this->typeFQCN ? TextType::class : 'text', [], Guess::LOW_CONFIDENCE);
Copy link
Member

Choose a reason for hiding this comment

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

Now I saw the test I'm wondering whether guessing TextType (even with low confidence) is correct. From what I saw in https://symfony.com/doc/current/form/type_guesser.html it's possible to just return; if we can't (or shouldn't) make a guess at all. Do you think it'd be more appropiate?

Copy link
Contributor Author

@BoShurik BoShurik Apr 13, 2018

Choose a reason for hiding this comment

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

I copy behaviour from a symfony/doctrine-bridge. Looks like in the first versions guessType returns only TypeGuess instance (based on phpDoc). I think now we can return nulls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@malarzm Do you want me to change it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please

Copy link
Member

Choose a reason for hiding this comment

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

Also if you could squash commits later that'd be awesome :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@BoShurik
Copy link
Contributor Author

@malarzm ping! Is there any other things that prevent merging?

@alcaeus alcaeus requested a review from malarzm April 19, 2018 14:04
@malarzm
Copy link
Member

malarzm commented Apr 19, 2018

Sorry, had this email starred but did not get back to it in time. Looks good to me! @alcaeus are we couting this as a bugfix and releasing 3.4.3 or will this wait for 3.5.0?

@malarzm
Copy link
Member

malarzm commented Apr 19, 2018

FWIW I'm fine with relasing 3.4.3 with this fix only as we do not get that many issues to fix.

@alcaeus
Copy link
Member

alcaeus commented Apr 19, 2018

Bugfix it is.

@malarzm malarzm added this to the 3.4.3 milestone Apr 19, 2018
@malarzm malarzm added the bug label Apr 19, 2018
@malarzm malarzm merged commit d40fc1a into doctrine:master Apr 19, 2018
@malarzm
Copy link
Member

malarzm commented Apr 19, 2018

@BoShurik enjoy 3.4.3 ;)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants