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

Introduce sanity checks for IDs and fix overriding its type #1190

Merged
merged 4 commits into from
Aug 11, 2015

Conversation

malarzm
Copy link
Member

@malarzm malarzm commented Aug 1, 2015

Fixes #857 (and by the way fixes #1178), closes #1183 and closes #819.

}

// \MongoId::isValid($idValue) was introduced in 1.5.0 so it's no good
if ($class->generatorType === ClassMetadata::GENERATOR_TYPE_AUTO && $idValue !== null && ! preg_match('/^[0-9a-f]{24}$/', $idValue)) {
Copy link
Member

Choose a reason for hiding this comment

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

Technically, this could happen with any generator strategy, including custom. It probably would be best if the generator itself could handle this check, including on whether to ignore it, throw an exception or quietly change the value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Partially I agree but since AUTO strategy is the default one its misuse is the most problematic (see #819 or tests I needed to fix in this PR like this one), I think we can safely assume that if somebody is changing strategy he is clever enough to pass correct id (or will have his own checks in the type).

@alcaeus
Copy link
Member

alcaeus commented Aug 2, 2015

To be honest, I don't know why we have the generatorOptions type at all. In the end, it does exactly the same as the field mapping but it just introduces complexity and mistakes along the way. I'm not sure about BC here, but IMO the best option would be to drop generatorOptions.type, always use type from the field mapping and setting it to id or custom_id depending on the generator strategy only if no type was given. As it is now, writing a generator that generates a MongoId still requires specifying a generatorOptions.type mapping because the field mapping of id will be overwritten (which, incidentally, needed a trip through the debugger to figure out why it wouldn't work when I first tried it without generatorOptions.type).

Either way we do this, we need to write some documentation about this feature.

@malarzm
Copy link
Member Author

malarzm commented Aug 2, 2015

I'm not sure about BC here, but IMO the best option would be to drop generatorOptions.type

Maybe trigger deprecation?

writing a generator that generates a MongoId

What is the point of such generator?

Either way we do this, we need to write some documentation about this feature.

I'll create a ticket for this later

@alcaeus
Copy link
Member

alcaeus commented Aug 3, 2015

What is the point of such generator?

The use case is a bit special: we needed predictable identifiers to trigger upserts but wanted to keep MongoId values. So we assigned a specific time identifier (the first 4 bytes of MongoId) and added a hash based on document settings. Weird, but it works ;)

@malarzm
Copy link
Member Author

malarzm commented Aug 10, 2015

@alcaeus I've commited a change we discussed on IRC :)

@alcaeus
Copy link
Member

alcaeus commented Aug 11, 2015

@malarzm Looks good. This way we ensure using MongoId with AUTO strategy but don't overly complicate the mapping. I've also created #1197 to track documentation changes.

malarzm added a commit that referenced this pull request Aug 11, 2015
Introduce sanity checks for IDs and fix overriding its type
@malarzm malarzm merged commit 23aca0f into doctrine:master Aug 11, 2015
@malarzm malarzm deleted the id-checks branch August 11, 2015 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants