Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Use hydrator variable only when hydrator variable is assigned #6949

Closed
wants to merge 2 commits into from
Closed

Use hydrator variable only when hydrator variable is assigned #6949

wants to merge 2 commits into from

Conversation

dkemper
Copy link
Contributor

@dkemper dkemper commented Nov 29, 2014

The hydrator variable is not in use before $this->getHydratorFromName() is called. i just fixed the structure.

@Ocramius
Copy link
Member

@dkemper so no logical changes?

@dkemper
Copy link
Contributor Author

dkemper commented Nov 29, 2014

No logical changes.

$hydratorOrName
));
}
if (!$hydrator instanceof Hydrator\HydratorInterface) {
Copy link
Member

Choose a reason for hiding this comment

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

This won't be reached when I call prepareAndInject(0, $fieldset)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, but in my case it throws a "Notice" undefined variable hydrator. So i will add a !isset($hydrator). Are you ok with it?

Copy link
Member

Choose a reason for hiding this comment

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

I think I'll have to check it out locally and see existing tests. Should be fine as-is

@dkemper
Copy link
Contributor Author

dkemper commented Nov 29, 2014

I will push my test.

@Ocramius Ocramius self-assigned this Nov 30, 2014
@Ocramius Ocramius added this to the 2.3.4 milestone Nov 30, 2014
@Ocramius
Copy link
Member

I'll merge as-is, seems like there was a bugfix after all ;-)

Ocramius added a commit that referenced this pull request Nov 30, 2014
Ocramius added a commit that referenced this pull request Nov 30, 2014
Ocramius added a commit that referenced this pull request Nov 30, 2014
Ocramius added a commit that referenced this pull request Nov 30, 2014
@Ocramius Ocramius closed this in 5b557b2 Nov 30, 2014
@Ocramius
Copy link
Member

@dkemper merged, thanks!

master: 5b557b2
develop: fe63bd2

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

Successfully merging this pull request may close these issues.

2 participants