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

EMongoFile: missing setFile afterInsert #122

Closed
yii-joblo opened this issue Sep 26, 2013 · 13 comments
Closed

EMongoFile: missing setFile afterInsert #122

yii-joblo opened this issue Sep 26, 2013 · 13 comments

Comments

@yii-joblo
Copy link

If EMongoFile->insert() is executed, the _file=null (or points to a maybe not existing CUploadedFile)

But I expect the _file property is set to the MongoGridFSFile that was inserted.

So you should call $this->setFile in the insert method after storeFile.

if($this->getCollection()->storeFile($this->getFilename(), $this->getRawDocument())){

             //add setFile
              $this->setFile($this->getCollection()->get($this->_id));                                

            $this->afterSave();
            $this->setIsNewRecord(false);
            $this->setScenario('update');
            return true;
        }

Not a good performance, but necessary for more operations after inserting (maybe for export, converting ...) a model.

@Sammaye
Copy link
Owner

Sammaye commented Sep 26, 2013

Hmm let me check this out I might be able to associate the database file etc without having to do another round trip.

@Sammaye
Copy link
Owner

Sammaye commented Sep 27, 2013

I did find a way of doing this but I reverted it to do it the way you showed minaly because it was actually more performant to just query again for the result of a single row that is likely to be in working set.

@yii-joblo
Copy link
Author

Maybe it's better to modify the getFile() method.

If I call getFile() and the model is in the mongodb (means $this->_id is assigned) I expect to work with the methods of MongoGridFSFile: 'write' to the filesystem, getResource() for streaming ...

So maybe like below:

public function getFile() { if(!empty($this->_id) && !(($this->_file instanceof MongoGridFSFile))) { $this->_file=$this->getCollection()->get($this->_id); }
   return $this->_file;
 }

If you do so, you have to change the getBytes() and getFilename() methods to use $this->getFile() instead of $this->_file;

public function getSize(){ return $this->getFile()->getSize(); }

public function getFilename(){
$file = $this->getFile();
if(empty($file)
throw new CException('_file not assigned');

if($file instanceof MongoGridFSFile)
    return $file->getFilename();
    return $file->getTempName();
}

... getBytes() implementation ... analogous.

@yii-joblo
Copy link
Author

But the best would be to implement the magic methods __call, __get, ... to work directly
with the driver methods/properties if $this->getFile() (implemented like above) is a MongoGridFSFile.

In this case your implementation is always uptodate with the driver and I can do like:

$mongofile->write(), $mongofile->getResource() for streaming

@Sammaye
Copy link
Owner

Sammaye commented Sep 27, 2013

tbh the getFilename etc should be getFile() anyway for KISS and DRY programming reasons so that has been done now.

About the function use, I have an idea, just change the __call:

/**
 * Magic will either call a function on the file if it exists or bubble to parent
 * @see EMongoDocument::__call()
 */
public function __call($name,$parameters){
    if($this->getFile() instanceof MongoGridFSFile && method_exists($this->getFile(), $name))
        return call_user_func_array(array($this->getFile(), $name), $parameters);
    return parent::__call($name,$parameters);
}

That should do it.

Then that solves adding that to getFile(), instead you can just use the EMOngoFile straight off

@yii-joblo
Copy link
Author

Nice, we had the same idea ;-)

@Sammaye
Copy link
Owner

Sammaye commented Sep 27, 2013

Oh yeah I see that's what you meant by your second comment lol

@yii-joblo
Copy link
Author

Missing the change in getFile() in your commit

public function getFile() { if(!empty($this->_id) && !(($this->_file instanceof MongoGridFSFile))) { $this->_file=$this->getCollection()->get($this->_id); }
   return $this->_file;
 }

@yii-joblo yii-joblo reopened this Sep 27, 2013
@Sammaye
Copy link
Owner

Sammaye commented Sep 27, 2013

With the change in __call and insert it shouldn't be needed? Can you give a case where it is?

@yii-joblo
Copy link
Author

oh, didn't see the change: setfile in insert.

But with the getFile() from above you don't need to setfile(...) in insert (or maybe update too??).
It's better performance on insert without $this->setFile($this->getCollection()->get($_id));.
The $this->getCollection()->get($this->_id) will only be executed, if I need to work with the model file methods after inserting, otherwise not.

The getFile() implemention from above ensures always to work with the gridFS file if it has an _id, independent of inserted, updated,... before.

@Sammaye
Copy link
Owner

Sammaye commented Sep 27, 2013

True that actually, no need to get the file if you don't need to work on it again.

Updating will just work no need to do anything with that, inserting is the problem but yeah I see what you mean. I'll change it

@Sammaye
Copy link
Owner

Sammaye commented Sep 27, 2013

That should do it: 291f11e

@yii-joblo
Copy link
Author

Thanks, I will test it.

pahanini pushed a commit to pahanini/MongoYii that referenced this issue Nov 2, 2013
pahanini pushed a commit to pahanini/MongoYii that referenced this issue Nov 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants