Allow increment strategy for not defined types

[uLow]

Q	A
Type	bug/improvement
BC Break	no

Summary

As decimal128 is not provided by doctrine types, it may be created as custom type. But even if mongo allows to use increment strategy with decimal128, doctrine throws an exception, that increment storage strategy is not allowed. Only doctrine restriction is to deal with. No other implementation is needed.

[alcaeus]

As decimal128 is not provided by doctrine types, it may be created as custom type.

Yes and no - we should rather ship the new type by default, allowing users to use it without implementing custom types. This would also allow us to not allow the increment strategy for all types by default but rather allow it for the new decimal128 type.

Would you be up for adding the decimal128 type to ODM 2.0?

[uLow]

@alcaeus I'm not that confident to do that. Just started to work with mongo. But I can't get the idea of not existing of customized storage types and therefore having such restriction.
Can we wait for this PR to be merged or use custom fork until 2.0?

[jmikola]

Note that in order to increment a Decimal128 object one will need to convert it to a string, calculate the increment (likely using bcadd() from BCMath), and convert the result back to a Decimal128 object¹.

The driver's handling of Decimal128 is currently limited to conversion to/from a string. It does not implement the necessary do_operation object handler to allow math operations on these objects. PHPC-847 is tracking implementation of the compare_objects handler to allow two Decimal128 objects to be compared with one another. While that's likely simpler than implementing do_operation, it is still not a trivial task.

On a related note, Internal operator overloading and GMP improvements was implemented in PHP 5.6 (php/php-src#342). GMP only supports integer math. The RFC did allude to supporting BCMath but I don't think that was ever implemented. That is likely because the bcmath extension does not provide any objects with which to implement the handlers -- unlike gmp, which provides both a procedural API and an object-oriented API using the GMP class.

The relevant code for actually handling the "increment" storage strategy is in PersistenceBuilder:

mongodb-odm/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php

Lines 149 to 151 in e7c926b

	if ($new !== null && isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadataInfo::STORAGE_STRATEGY_INCREMENT) {
	$operator = '$inc';
	$value = Type::getType($mapping['type'])->convertToDatabaseValue($new - $old);

@uLow: That will require some modifications to actually support the feature you're asking for. Simply adding STORAGE_STRATEGY_INCREMENT to the list of allowed strategies isn't going to cut it.

Thinking more generally, the best way to implement this may be to allow types (either custom or ODM's built-in types) to implement some interface that provides a method to calculate an increment.This would basically be a limited form of operator overloading used expressly for ODM's increment strategy. If implemented, a future Decimal128 type in ODM could then implement this interface and utilize BCMath internally as it wishes.

@alcaeus: If that seems reasonable, perhaps you can quote/paraphrase this comment and create an issue to track the feature request for a future ODM version.

---

1. Alternatively, a future ODM type for Decimal128 might convert it to a string for its PHP representation to save users the trouble of doing so themselves. This would be similar to how BSON dates are converted to a PHP DateTime when set on entities.

[alcaeus]

Given the excellent explanation by @jmikola, I'm closing this one here. Just allowing the increment strategy will do you no good since we'd also have to use bcmath to calculate the difference to send to the database server. Without that, you might as well not use the Decimal128 type and stick with 64-bit numbers.

I've created #1835 to track the feature request to add a new Decimal128 type which would also incorporate the logic to properly calculate the difference when using an increment strategy. Given our current progress on 2.0 and the timeline, I've scheduled it for inclusion in a 2.1 release.