"Notice: Undefined offset: 0" when there is no ID Column defined #6412
Conversation
|
@JarJak metadata without identifier isn't really valid metadata - is this for an embeddable? Can a test be added? |
|
@Ocramius I agree that metadata without identifier is invalid, however if you throw nice Exception than this Notice, then this mistake would be much easier to debug and fix. I will add test later. |
When you forget about defining the ID/PK Column, then this ugly Notice appear. Now it will throw nice Exception.
|
@Ocramius I've updated PR with two nearly identical tests (I don't know where exactly I should put my test, so I've created them in 2 places). This change might be a BC Break if someone has error reporting turned off (before it would silently fail instead of throwing an exception), however since master even switched to PHP7 I think this minor BC Break wouldn't be a problem. |
| */ | ||
| public static function noIdDefined($entity) | ||
| { | ||
| return new self('No ID defined for entity '.$entity); |
There was a problem hiding this comment.
Please add spaces before and after the concatenation operator ('No ID defined for entity ' . $entity)
| /** | ||
| * @author JarJak | ||
| */ | ||
| class DDC6412Test extends OrmTestCase |
There was a problem hiding this comment.
I'd say it's better to leave this check in ClassMetadataTest.php only, then we keep it together with the other mapping assertions
| @@ -1803,6 +1803,10 @@ public function getSingleIdentifierFieldName() | |||
| if ($this->isIdentifierComposite) { | |||
| throw MappingException::singleIdNotAllowedOnCompositePrimaryKey($this->name); | |||
| } | |||
|
|
|||
| if ( ! isset($this->identifier[0])) { | |||
There was a problem hiding this comment.
Can you also mention this change in the docblock? We currently have @throws MappingException If the class has a composite primary key. but would be good to say something like @throws MappingException If the class doesn't have an identifier or it has a composite primary key.
@JarJak It's not really considered a BC Break since we already expected that type of exception. It was actually something that we missed. Thanks for addressing it 😉 |
|
@lcobucci I polished the code according to your tips, thanks :) |
|
@lcobucci no, I think this is fine with |
When you forget about defining the ID/PK Column, then this ugly Notice appear.
This PR is not ready for merge, but shows where the problem exist.
Could you fix this or tell me the way this Pull Request would be acceptable? Thanks.
How to reproduce?
Remove ORM\Id annotation, then try to regenerate entities.