Silence E_NOTICE warning: "Undefined index". #298
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DCOM-216 We use Jira to track the state of pull requests and the versions they got |
|
@fredrik-w the fix is invalid. A proxy expects ALL fields of the PK to be set. Can you provide the case where this is happening? The problem is not the factory, and the notice is alright here (could be changed into an exception eventually, but only if really needed, since it's a performance sensitive part of the ORM/ODM). |
| @@ -118,7 +118,8 @@ public function getProxy($className, array $identifier) | |||
| $proxy = new $fqcn($definition->initializer, $definition->cloner); | |||
|
|
|||
| foreach ($definition->identifierFields as $idField) { | |||
| $definition->reflectionFields[$idField]->setValue($proxy, $identifier[$idField]); | |||
| $value = isset($identifier[$idField]) ? $identifier[$idField] : null; | |||
There was a problem hiding this comment.
null is not a valid PK field value
|
Well that explains a bit more. For some reason (copy/paste monster perhaps) I had two columns defined as the PK when it should have been only one. I feel like a proper exception thrown here would be better than an anonymous E_NOTICE. Can I just update my branch and replace the "fix" with throwing an OutOfBoundsException("Missing value for primary key {$idField} on {$className}"); |
|
@fredrik-w can you modify the PR accordingly? Adding a test is also necessary |
|
I've updated the PR; now a proper exception is thrown if the value for a PK is missing. |
| @@ -118,6 +118,9 @@ public function getProxy($className, array $identifier) | |||
| $proxy = new $fqcn($definition->initializer, $definition->cloner); | |||
|
|
|||
| foreach ($definition->identifierFields as $idField) { | |||
| if (!isset($identifier[$idField])) { | |||
| throw new \OutOfBoundsException("Missing value for primary key {$idField} on {$className}"); | |||
| } | |||
There was a problem hiding this comment.
Add a new empty line after this line
primary key is missing.
|
New exception class added, fixed coding style as well. |
Silence E_NOTICE warning: "Undefined index".
|
Thanks, the error message is much clearer now. However if i pull back to 2.3.3, this data in my case is tolerated. Suppose i have Entity A, B and C.
Now if i load It will have half of the reference-identifier |
Silence E_NOTICE warning: "Undefined index".
|
@Ocramius: This actually broke ODM's test suite recently (build 1236), since Edit: Sorry for bumping an old thread (just realized this is nearly a year old). Looks like ODM picked it up since this was included in the v2.4.2 release. |
This test was originally added alongside a ReferenceMany test in d1b0ed3, but the latter was removed in a2c3ddd when @jwage added tests for non-scalar identifiers. The ReferenceOne test likely should have been removed at that point, too. This fixes a build failure with Doctrine Common 2.4.2, due to doctrine/common#298. Therein, the common proxy classes no longer allow for null identifiers, even though they are technically valid in MongoDB.
|
Why is "not a value" an actual identifier? That's a conceptual mistake...
|
|
Filed under "not that you would but you could."
I just ended up deleting the test case, and I don't expect that any ODM users actually relied on this. I was just surprised to see this in a patch release, as throwing a new exception seemed like an API change more suited to a X.Y bump. |
|
@jmikola it is weird that
|
Running PHP with error reporting on for E_NOTICE this bit me. Fix is simple and straightforward, check if key is defined and use that value if so, otherwise use null.