-
Notifications
You must be signed in to change notification settings - Fork 11
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
Nette 2.4 compatibility #42
base: master
Are you sure you want to change the base?
Conversation
…e\Database (thanks @akyn84, @lubo)
@@ -72,7 +72,7 @@ function addFileManually($name, $chunk, $chunks); | |||
* @param type $chunk | |||
* @param FileUpload $file | |||
*/ | |||
function updateFile($name, $chunk, FileUpload $file = null); | |||
function updateFile($name, $chunk, FileUpload $file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akyn84 why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put this question conversely: is there special reason to have optional parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. :)
@@ -0,0 +1,162 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akyn84 what was wrong with original nette\database + MySQL implementation? https://github.com/jkuchar/MultipleFileUpload/tree/master/MultipleFileUpload/Model/NetteDatabase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nette\Database implementation was not in last release (1.1.0) donwloaded by composer thus I did not notice it is in master branch by Zdeněk Jurka. Probably it solve most of issues with nette 2.4? (Nette\Environment, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I check just right now and for sure it is not compatible with PHP 7 so I will continue in update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nette\Database implementation was not in last release use Nette\Environments, I am reverting to my orignal solution -> when it will be approved, i will refactor all Models but firs i need to confirm MySQL namespace (later it can be removed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace should be names as the target for the bridge. You you name namespace mysql, I would expect to that this driver will use mysql_* functions
$stmt->execute(); | ||
} | ||
$this->query("COMMIT WORK"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akyn84 mixing of spaces + tabs
*/ | ||
function updateFile($name, $chunk, FileUpload $file) | ||
{ | ||
$this->query("START TRANSACTION"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akyn84 why not use Nette database ->startTransaction() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
self::init(); | ||
$application = $container->getByType('Nette\Application\Application'); | ||
$application->onStartup[] = [__CLASS__, "handleUploads"]; | ||
$application->onShutdown[] = [__CLASS__, "cleanCache"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akyn84 This whole registration should be moved into Container Extension? Are you going to handle that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, tomorrow as proper nette DI Extension
} | ||
|
||
|
||
/** | ||
/**}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
self::setQueuesModel(new Model\SQLite3\Queues()); | ||
if (!self::$queuesModel) { // if nothing is set, setup sqlite model, which should work on all systems with MySQL | ||
$queuesModel = new Model\MySQL\Queues(self::$container, self::$container->getService("database.default")); | ||
self::setQueuesModel($queuesModel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akyn84 remove this magic default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
model inject as string namespace in constructor as in Registrator->getInterfaces() ?
MultipleFileUpload/Extension.php
Outdated
$initialize->addBody('MultipleFileUpload\MultipleFileUpload::init($this->getByType(?), $this->getParameters());', ['Nette\Http\IRequest']); | ||
$initialize->addBody('MultipleFileUpload\MultipleFileUpload::register($this->getService(?));', ['mfuStorage']); | ||
$initialize->addBody('$this->getService(?)->onStartup[] = [?, ?];', ['application', 'MultipleFileUpload\MultipleFileUpload', 'handleUploads']); | ||
$initialize->addBody('$this->getService(?)->onShutdown[] = [?, ?];', ['application', 'MultipleFileUpload\MultipleFileUpload', 'cleanCache']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
completely missing configuration of storage driver + UIs
}; | ||
$cacheStorage = new Nette\Caching\Storages\FileStorage(self::$uploadsTempDir); | ||
$structure = new Nette\Database\Structure($connection, $cacheStorage); | ||
$this->database = new Nette\Database\Context($connection, $structure); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whole Database\Context should be injected. IT is responsibility of DI container to create connection.
|
||
/** | ||
* Initialize MFU | ||
*/ | ||
public static function init() | ||
public static function init(Nette\Http\IRequest $request, Array $parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static methods from this class init(), register(), ... needs to be split into another class that will be created and registered in constructor. MFU will then take this object as a dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when this functionality is removed to separate class and injected in constructor, dependencies are missing in MultipleFileUpload::handleUploads called on startup of application. If handleUploads is dynamic and called on Nette\Application\Application->startUp[], class itself is initialized before attached to component (too soon). can be logic of MultipleFileUpload::handleUploads solved outside of class with completly independent service?
Hi, |
im accepting PR on this. we can make a conf call with @landrisek who is working on this |
} | ||
return self::$dibiConnection; | ||
$this->conection = $conection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the class variable is called connection
so it should be
$this->connection = $conection;
* @return \DibiConnection | ||
*/ | ||
function getConnection() | ||
public function __construct(string $tempDir, DibiConnection $conection) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only note: the latest version of dibi has Dibi\Connection instead of DibiConnection.. it would be nice to be compatible with latest dibi
Can you please give me a note how to setup current MFU from nette-2-4-compatibility branch? Latest commit I was able to setup with nette 2.4 is dev-nette-2-4-compatibility#e53eac88cabddb27ed1d8b3ba942ca353620d49e and I have to use following: bootstrap.php
and replace "->add(" with "->addHtml(" in MultipleFileUpload.php |
To be honest, I do not know as I do not use this code on Nette 2.4, when you figure it out (XDebug is your friend), clould you please post it here? |
thanks @akyn84 @lubo