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

DI is not found in MongoCollection using Phalcon 3.1 and PHP 7.1 #760

Closed
MaksSlesarenko opened this issue Apr 14, 2017 · 58 comments
Closed

Comments

@MaksSlesarenko
Copy link

MaksSlesarenko commented Apr 14, 2017

Getting error in MongoCollection after updating to Phalcon 3.1.2 and PHP 7.1

$file = File::findFirst()
$file->name = 'test';
$file->save();


A dependency injector container is required to obtain the services related to the ODM
                                                                                         
  [Phalcon\Mvc\Collection\Exception]                                                     
  A dependency injector container is required to obtain the services related to the ODM  
                                                                                         

Exception trace:
 () at ...vendor/phalcon/incubator/Library/Phalcon/Mvc/MongoCollection.php:76
 Phalcon\Mvc\MongoCollection->save()

Current workaround in model class

 public function save()
    {
        if (version_compare(PHP_VERSION, '7.1.0') >= 0) {
            $this->_dependencyInjector = \Phalcon\Di::getDefault();
            $this->_modelsManager = $this->_dependencyInjector->getShared("collectionManager");
            $this->_modelsManager->initialize($this);
        }

        return parent::save();
    }

Details

  • Phalcon Framework version: 3.1.2
  • Phalcon Incubator version: 3.0 & 3.1
  • PHP Version: 7.1.2
  • Operating System: Centos
  • Server: CLI
  • Other related info (Database, table schema): Mongo
@tkroll
Copy link

tkroll commented Apr 18, 2017

Related: https://forum.phalconphp.com/discussion/15927/mongocollection-wont-save

Class is ignoring $_reserved in getReservedAttributes(). Why?

This fix in the model or in MongoCollection.php fixes the above DI, saving problem.

public function getReservedAttributes()
{

    $reserved = parent::$_reserved;
    if(null === $reserved) {
        $reserved = [
            "_connection"=> true,
            "_dependencyInjector"=> true,
            "_source"=> true,
            "_operationMade"=> true,
            "_errorMessages"=> true,
            "_dirtyState"=> true,
            "_modelsManager" => true,
            "_skipped" => true,
            // last two to be complete
            "_reserved" => true,
            "_disableEvents" => true
        ];
        parent::$_reserved = $reserved;
    }
    return $reserved;
}

@ghost
Copy link

ghost commented May 2, 2017

Mine works when creating a new object:

// works fine

$user = new Users();
$user->email = "user@email.com";
$user->save(); 

But when updating an entry, I'm getting the same error:

// A dependency injector container is required to obtain the services related to the ODM

$user = Users::findFirst([[ "email" => "user@email.com" ]]);
$user->email = "updated@email.com";
$user->save(); 

I think it's related to issue #762. When saving, it stores even the private fields, including _dependencyInjector, causing the _dependencyInjector to become an empty object when retrieved using Users::findFirst().

@MaksSlesarenko
Copy link
Author

I've updated issue, error happens on update not insert

@Jurigag
Copy link
Contributor

Jurigag commented May 10, 2017

MongoCollection extends Phalcon\Mvc\Collection. Here is collection construction - https://github.com/phalcon/incubator/blob/master/Library/Phalcon/Mvc/MongoCollection.php#L186, here it sets dependencyInjector -
https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/collection.zep#L95

Not sure what happens, it should work. Any idea? What is returned after you do $file->getDI() ? This it returns anything? Or null?

It just looks like you don't have any Di created just $di = new Di(); should do it anywhere, but maybe try Di::setDefaulut($di);?

@MaksSlesarenko
Copy link
Author

@Jurigag look at @tkroll comment

@Jurigag
Copy link
Contributor

Jurigag commented May 10, 2017

For me on php 7.0.13 it works but i have:

Uncaught MongoDB\Driver\Exception\BulkWriteException: Mod on _id not allowed

From code like:

$robots = new Robots();
$robots->name = 'test';
$robots->create();
$robots = Robots::findFirst();
var_dump($robots->toArray());
$robots->name = 'test2';
$robots->save();

I guess when updating collection we should remove _id

@micheleangioni
Copy link
Contributor

micheleangioni commented May 11, 2017

@sergeyklay I'm having the same problem. Updating to PHP 7.1 and Phalcon 3.1.2 somehow broke an application where we are using Mongo.

I get A dependency injector container is required to obtain the services related to the ODM when updating records.

Instead, when deleting records I get

Error: Call to a member function notifyEvent() on array

/var/www/wiki/vendor/phalcon/incubator/Library/Phalcon/Mvc/MongoCollection.php:467
/var/www/wiki/vendor/phalcon/incubator/Library/Phalcon/Mvc/MongoCollection.php:352

I do not know whether @sergeyklay is the maintainer or someone else, but please this should have very high priority since Mongo currently is not usable.

Thanks!

UPDATE:

The workaround

        if (version_compare(PHP_VERSION, '7.1.0') >= 0) {
            $this->_dependencyInjector = \Phalcon\Di::getDefault();
            $this->_modelsManager = $this->_dependencyInjector->getShared("collectionManager");
            $this->_modelsManager->initialize($this);
        }

can also fixes the delete() method.

@Jurigag
Copy link
Contributor

Jurigag commented May 11, 2017

I can't reproduce it, i checked it on latest phalcon and incubator and it works fine, i have other error above.

@micheleangioni
Copy link
Contributor

micheleangioni commented May 11, 2017

It's strange you cannot reproduce it, since many people are having the same problem..

Are you using PHP 7.1? Because with PHP 7.0 it probably works fine (at least for me it was working fine with PHP 7.0).

UPDATE:
My current PHP version is 7.1.4
OS: Ubuntu 16.04
Incubator: 3.1.x

@Jurigag
Copy link
Contributor

Jurigag commented May 11, 2017

Yes, using php 7.1 i have:

Uncaught MongoDB\Driver\Exception\BulkWriteException: Mod on _id not allowed

Though i have other idea why you might have this issue, do you maybe have your own di? Which DI you use FactoryDefault or just Di?

@micheleangioni
Copy link
Contributor

Interesting question, though in the app I use the FactoryDefault, in the tests it's the Phalcon\Test\UnitTestCase class that sets the DI with $di = new Di(); where DI is use Phalcon\Di;, after a Di::reset();, line 63

@Jurigag
Copy link
Contributor

Jurigag commented May 11, 2017

Then idk, i just don't have it really. Weird issue, and this problem you have in both, like app and tests?

@micheleangioni
Copy link
Contributor

It seems both, but tomorrow I'll make some more detailed checks!

@Jurigag
Copy link
Contributor

Jurigag commented May 11, 2017

I just don't have this error, which is weird, you could try doin something like Di::setDefault($di) but i doubt it will fix it. dependencyInjector is set in parent class. Further it's not lost anywhere as you trying to talk. Though surely there is problem with those reserved attributes, maybe it's causing this di error, not sure.

@micheleangioni
Copy link
Contributor

micheleangioni commented May 12, 2017

@Jurigag ok, it seems the errors are present only in tests.
So the problem seems to be something related with the Incubator itself, between the Mongo and Test modules?

In the parent class I have $di = Di::getDefault();. Even if I change it to $di = new \Phalcon\DI\FactoryDefault(); and/or add Di::setDefault($di); nothing changes.

I agree that's weird.. but several people seem to have the same problem.
In the parent class I have

// [...]
use Phalcon\Db\Adapter\MongoDB\Client;
use Phalcon\Test\UnitTestCase as PhalconTestCase;

abstract class WebTestCase extends PhalconTestCase
{

// [...]

 public function setUp()
    {
        parent::setUp();

        // Load any additional services that might be required during testing
        $di = Di::getDefault();

        // [...]

        /**
         * Database connection is created based in the parameters defined in the configuration file
         */
        $di->setShared('mongo', function () {
            $dsn = 'mongodb://' . env('DB_HOST', 'localhost');

            $mongo = new Client($dsn);

            return $mongo->selectDatabase(env('DB_NAME'));
        });

        // Collection Manager is required for MongoDB
        $di->setShared('collectionManager', function () {
            return new Manager();
        });

        // [...]
    }

// [...]

}

But before upgrading to PHP 7.1 and Phalcon 3.1.2 it was working without problems.
If something comes to your mind that I could do to help you reproduce the error, please let me know. Unfortunately I cannot open source the code.

phalcon/incubator: v3.1.1

@micheleangioni
Copy link
Contributor

I can confirm I have this problem also in the web (i.e. not in tests).

Here is the error when I try to save a model:

A dependency injector container is required to obtain the services related to the ODM`

#0 /var/www/[...]: Phalcon\Mvc\MongoCollection->save()
#1 /var/www/[...]: [...]->[...]()
#2 [internal function]: [...]->requested(Object(Phalcon\Events\Event), Object([...]), Object([...]))
#3 [internal function]: Phalcon\Events\Manager->fireQueue(Array, Object(Phalcon\Events\Event))
#4 /var/www/[...](143): Phalcon\Events\Manager->fire('[...]', Object([...]), Object([...]))
#5 [internal function]: [...]
#6 [internal function]: Phalcon\Dispatcher->callActionMethod(Object([...]), '[...]', Array)
#7 [internal function]: Phalcon\Dispatcher->_dispatch()
#8 [internal function]: Phalcon\Dispatcher->dispatch()
#9 /var/www/[...]/public/index.php(35): Phalcon\Mvc\Application->handle()
#10 {main}

Is there any progress about this?

@Jurigag
Copy link
Contributor

Jurigag commented May 15, 2017

If someone will post full script to reproduce then sure i guess.

@tkroll
Copy link

tkroll commented May 16, 2017

Can someone test 7.1.5.
RE: Fixed bug #74188 (Null coalescing operator fails for undeclared static class properties).

This is interesting as many static props were undeclared.

@micheleangioni
Copy link
Contributor

@tkroll still same problem with

PHP 7.1.5-1+deb.sury.org~xenial+1

@tkroll
Copy link

tkroll commented May 16, 2017

Boo. Thanks for trying. It was a shot in the dark.

@Jurigag
Copy link
Contributor

Jurigag commented May 16, 2017

It works for me on 7.1.0 or 7.0.0, i just have error what i wrote above, someone needs to create repo with full code to reproduce.

@Jurigag
Copy link
Contributor

Jurigag commented May 17, 2017

#768

As you see we don't have error with this Di thing, we need script to reproduce.

@Jurigag
Copy link
Contributor

Jurigag commented May 23, 2017

@baychae why you thumb down? If you have script to reproduce then post it please.

@micheleangioni
Copy link
Contributor

@Jurigag @sergeyklay I have now a public repo that shows this problem.

PHP 7.0 : just 1 test failing
PHP 7.1 : ALL Mongo tests failing

PHP 7.1 with FIX : just 1 test failing

Conclusions:

  1. Without the fix, in PHP 7.1 all tests fail, in PHP 7.0 only one
  2. Locally no tests are failing in PHP 7.0 and in PHP 7.1 with FIX, could you take a look and try to understand why that test is failing on Travis? Thanks

Imho the bug level is Medium or High.

@Jurigag
Copy link
Contributor

Jurigag commented May 23, 2017

@micheleangioni i have this thing about _id too, i think it's about mongodb/mongodb extension version, that from some version they disabled option to update _id field. This is why you don't have about this _id update locally. Also which FIX you mean for php 7.1? Those notifyEvent are most likely other issue.

@micheleangioni
Copy link
Contributor

@Jurigag it might be, but in the failing test I am NOT updating the _id field, that's the point!
I am updating another field.

We have to understand how to fix both problems, because at the moment this Mongo library is not usable :(

@micheleangioni
Copy link
Contributor

Hi @Jurigag ,
I see you are fixing the notifyEvent() bug/problem in phalcon/cphalcon/pull/12862, looking forward for the fix to be complete :)

About the _id problem, is there a PR o fix in progress?
Thanks!

@aliel
Copy link

aliel commented Jun 10, 2017

About the _id problem, is there a PR o fix in progress?
Thanks!

+1

@Jurigag
Copy link
Contributor

Jurigag commented Jun 12, 2017

phalcon/cphalcon#12891 waiting for merging this, then i will change build script because currently incubator uses latest stable phalcon(we need to use source and zephir) and will fix this _id issue and check if tests are passing.

@Jurigag
Copy link
Contributor

Jurigag commented Jun 20, 2017

Okay i merged mod on _id fix. Though we don't have update method at all in Phalcon\Mvc\MongoColection which seems weird. All should work now. If someone who is using mongo in their app could check Latest phalcon(3.2) and incubator on 3.2.x branch(it's not tagged release yet) to check if everything works it could be good. Then i will merge this mod on _id fix to master if everything is fine, if not will try to fix other stuff and then new release.

@Jurigag Jurigag closed this as completed Jun 20, 2017
@tkroll
Copy link

tkroll commented Jun 20, 2017

Just did:
git clone https://github.com/phalcon/cphalcon
cd cphalcon/build
sudo ./install --phpize /usr/bin/phpize7.1 --php-config /usr/bin/php-config7.1

then

composer require phalcon/incubator:^3.2

Got this error:

PHP Fatal error: Uncaught Phalcon\Mvc\Collection\Exception: A dependency injector container is required to obtain the services related to the ODM in /var/www/outsmart/shared/utils/incubator/Library/Phalcon/Mvc/MongoCollection.php:77

@Jurigag
Copy link
Contributor

Jurigag commented Jun 20, 2017

Script to reproduce?

@Jurigag Jurigag reopened this Jun 20, 2017
@tkroll
Copy link

tkroll commented Jun 20, 2017

$u = User::findByEmail('test@test.com');
$u->helloThere = 1;
echo $u->email, PHP_EOL;
$u->save();

My User class extends \Phalcon\Mvc\MongoCollection. It echos correctly.

@Jurigag
Copy link
Contributor

Jurigag commented Jun 20, 2017

Hmmmmm. Could you try other database? It's possible that you just have in mongo _dependencyInjector(and other saved) key saved.

I guess we will need now to change code to not set those reserved attributes if they returned from mongo database.

@tkroll
Copy link

tkroll commented Jun 20, 2017

Oh snap! Good call.

Did a quick:

db.user.update( {}, { $unset: { "_dependencyInjector":1, "_modelsManager":1, "_source":1, "_operationMade":1, "_dirtyState":1, "_connection":1, "_errorMessages":1, "_skipped":1 } }, {multi:1} )
And we appear to be back in business!

@Jurigag
Copy link
Contributor

Jurigag commented Jun 20, 2017

Yea now the question is:

  • should people just update their databases to unset those keys
  • or we should unset them in framework after find? - though it's stupid and adds overhead

@tkroll
Copy link

tkroll commented Jun 20, 2017

I'd think outside the framework. It's a bit of work to do every time.

This worked for me. No warranty on this.

Script for removing reservered properties from Mongo:
https://gist.github.com/tkroll/53fa3e9151d9232e7399e313b503dc97

@Jurigag
Copy link
Contributor

Jurigag commented Jun 20, 2017

Okay, so now i will close it, let me know if anything other occur, also there is this - #794 if you could comment here it could be good.

@Jurigag Jurigag closed this as completed Jun 20, 2017
@micheleangioni
Copy link
Contributor

micheleangioni commented Jul 4, 2017

@Jurigag I'm still having problems.
Please take a look at this Travis build

I am using the last Incubator version

Line 1070: - Installing phalcon/incubator (dev-master 4913dc0): Cloning 4913dc021f

Any clue? Thanks

Update
The MongoCollection.php class has still the offending line. Shouldn't this have been fixed? :/

@Jurigag
Copy link
Contributor

Jurigag commented Jul 4, 2017

It's fixed line above with unset($data['_id']);

@micheleangioni
Copy link
Contributor

@Jurigag ok now I get it, the PR has been merged in the 3.2.x branch but NOT in the Master branch. Is that really ok? Shouldn't be merged in both?

@Jurigag
Copy link
Contributor

Jurigag commented Jul 7, 2017

It wasn't? Oh i must forgot it. Let me merge it.

Already released.

@micheleangioni
Copy link
Contributor

micheleangioni commented Jul 7, 2017

Thanks :)

I conferm the build is now ok.

@Jurigag
Copy link
Contributor

Jurigag commented Jul 7, 2017

Great, happy to hear, also i would like to know your opinion about #794

wolftankk added a commit to wolftankk/incubator that referenced this issue Oct 9, 2017
wolftankk added a commit to wolftankk/incubator that referenced this issue Oct 10, 2017
wolftankk added a commit to wolftankk/incubator that referenced this issue Oct 10, 2017
wolftankk added a commit to wolftankk/incubator that referenced this issue Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants