Change private visibility to protected

[tux-rampage]

This library uses private almost everywhere. This excessive private usage makes the ODM very unflexible.

Because of #304 I had to implement a custom document persister. In order to avoid code duplication, I tried to extend the default document persister, which became a pain. I had to work around the private everywhere.

My code basically looks like this:

        /* @var $dm DocumentManager */
        $classMeta = $dm->getClassMetadata(entities\Product::class);
        $uow = $dm->getUnitOfWork();

        // Use the unit of work state to create the persistence builder
        $factory = function() use ($dm, $classMeta) {
            $pb = $this->getPersistenceBuilder();
            return new persistence\MyShardedDocumentPersister($pb, $dm, $this->evm, $this, $this->hydratorFactory, $classMeta);
        };

        // Properties are private so we have to use a closure to gain access
        $factory = $factory->bindTo($uow, UnitOfWork::class);
        $uow->setDocumentPersister(entities\Product::class, $factory());

And the persister:

namespace myapp\persistence;

use Doctrine\ODM\MongoDB\Persisters\DocumentPersister;

class MyShardedDocumentPersister extends DocumentPersister
{
    /**
     * @var callable
     */
    protected $privateGetter = null;

    /**
     * Work around for private properties
     *
     * @param string $property
     * @return mixed
     */
    protected function _get($property)
    {
        if (!$this->privateGetter) {
            $closure = function($property) {
                return $this->{$property};
            };

            $this->privateGetter = $closure->bindTo($this, DocumentPersister::class);
        }

        $closure = $this->privateGetter;
        return $closure($property);
    }

    /**
     * Executes a single upsert in {@link executeInserts}
     *
     * @param array $data
     * @param array $options
     * @param int $shardKey
     */
    protected function executeUpsert(array $data, array $options, $shardKey)
    {
        // Criteria must contain the shard key!
        $options['upsert'] = true;
        $criteria = [
            '_id' => $data['$set']['_id'],
            'shardKey' => $shardKey,
        ];

        // Do not update the shard key
        unset($data['$set']['_id'], $data['$set']['shardKey']);

        // Do not send an empty $set modifier
        if (empty($data['$set'])) {
            unset($data['$set']);
        }

        /* If there are no modifiers remaining, we're upserting a document with
         * an identifier as its only field. Since a document with the identifier
         * may already exist, the desired behavior is "insert if not exists" and
         * NOOP otherwise. MongoDB 2.6+ does not allow empty modifiers, so $set
         * the identifier to the same value in our criteria.
         *
         * This will fail for versions before MongoDB 2.6, which require an
         * empty $set modifier. The best we can do (without attempting to check
         * server versions in advance) is attempt the 2.6+ behavior and retry
         * after the relevant exception.
         *
         * See: https://jira.mongodb.org/browse/SERVER-12266
         */
        if (empty($data)) {
            $retry = true;
            $data = [
                '$set' => [
                    '_id' => $criteria['_id'],
                    'shardKey' => $criteria['shardKey'],
                ]
            ];
        }

        $collection = $this->_get('collection');

        try {
            $collection->update($criteria, $data, $options);
            return;
        } catch (\MongoCursorException $e) {
            if (empty($retry) || strpos($e->getMessage(), 'Mod on _id not allowed') === false) {
                throw $e;
            }
        }

        $collection->update($criteria, array('$set' => new \stdClass), $options);
    }

    /**
     * Executes all queued document upserts.
     *
     * Queued documents with an ID are upserted individually.
     * If no upserts are queued, invoking this method is a NOOP.
     *
     * This method respects the shard key!
     *
     * @param array $options Options for batchInsert() and update() driver methods
     */
    public function executeUpserts(array $options = array())
    {
        if (!($queuedUpserts = $this->_get('queuedUpserts'))) {
            return;
        }

        $pb = $this->_get('pb');
        $class = $this->_get('class');

        $parent = function($document, $options, $oid) {
            $this->handleCollections($document, $options);
            unset($this->queuedUpserts[$oid]);
        };
        $parent = $parent->bindTo($this, DocumentPersister::class);
        $parentHandler = function($oid) { unset($this->queuedUpserts[$oid]); };
        $parentHandler = $parentHandler->bindTo($this, DocumentPersister::class);

        foreach ($queuedUpserts as $oid => $document) {
            $data = $pb->prepareUpsertData($document);
            $shardKey = $class->reflFields['shardKey']->getValue($document);
            // The entity id is the shard key

            // Set the initial version for each upsert
            if ($class->isVersioned) {
                $versionMapping = $class->fieldMappings[$this->class->versionField];
                if ($versionMapping['type'] === 'int') {
                    $nextVersion = max(1, (int) $class->reflFields[$class->versionField]->getValue($document));
                    $class->reflFields[$class->versionField]->setValue($document, $nextVersion);
                } elseif ($versionMapping['type'] === 'date') {
                    $nextVersionDateTime = new \DateTime();
                    $nextVersion = new \MongoDate($nextVersionDateTime->getTimestamp());
                    $class->reflFields[$class->versionField]->setValue($document, $nextVersionDateTime);
                }
                $data['$set'][$versionMapping['name']] = $nextVersion;
            }

            try {
                $this->executeUpsert($data, $options, $shardKey);
                $parent($document, $options, $oid);
            } catch (\MongoException $e) {
                $parentHandler($oid);
                throw $e;
            }
        }
    }
}

[malarzm]

Making things protected means we have to keep BC for them only because somebody could have extended the class so I'm not in favour of this solution. @jmikola I remember you had some plans for sharding, orchestration and PRs we have for 1.x for this - maybe let's make a schedule for the feature? /cc @alcaeus

[tux-rampage]

Well that depends in how BC support is promised. I'd not suspect protected members and methods to be safe for BC.

But I got your point and if it's Doctrine's policy to provide BC for protected members so be it then. And since we've got php 5.5 there are nice tricks to work around strict visibilities without using reflections.

There are some cases where people need to change Doctrine's behavior more deeply for a certain (maybe very rare) use case. That would not be worth making it a feature. I have to admit that the sharding example is a bad one for a rare use case.

[alcaeus]

Yes, the sharding example is the worst possible case you could've picked for arguing for protected support. I don't think there's another feature that has three open, unfinished PRs sitting around (#325, #691, #1025) ;)

That being said, I also disagree with opening up a bunch of properties and methods for the sake of being extendable. Not every class contained in the package is meant to be extended or replaced.

Since you need sharding support, how about giving #1025 a test run before being the next person to start from scratch? From the looks of it it is in a state you can test it (with the usual precautions about using non-production-grade software in production environments), and I would assume @clslrns appreciates any feedback he gets. Hopefully by the time 1.x comes around we won't need to have this discussion about making the DocumentPersister extendable to support sharding.

[malarzm]

Closing the ticket as private visibility will not be changed blindly, let's keep discussion about sharding in sharding-related threads. If we come to any conclusion internally on how we want to proceed I'll create new meta issue for that.