Sf5 ?
ErnadoO opened this issue · 35 comments
Hello boys
Any plans to support Symfony 5 and/or when?
Thanks
Hi
It is planned to support Symfony 5 - but it's probably a bit of a mission.
Didn't look into it so far, but it's on our list. Can't tell you a date though.
You're happily welcome to provide a PR if you like. :)
@bytehead I'm happy to have a look at a PR for SF 5, but in my initial attempts to run the test suite against 4.4 many tests fail because of the signature change for the constructor of Symfony/Component/HttpFoundation/File/UploadedFile.php which between 4.0 and 4.1 went from accepting 6 arguments to only accepting 5 - dropping the size argument.
In 4.x there are attempts made to accommodate this change, but in some cases the parameter types now no-longer match the type hints so tests are failing, and the code which tried to resolve this is completely gone from 5.x.
Thus my question - how do you want to handle this? Given that SF 3.4 is still supported (active support for 11 months, security for 23 months) any change needs to support both the 5 and 6 parameter options. My initial thought is (for example on line 114 of MooUploadController)
if(Kernel::VERSION_ID < 40100) {
$file = new UploadedFile($tempFile, $uploadFileName, null, null, null, true);
} else {
$file = new UploadedFile($tempFile, $uploadFileName, null, null, true);
}
Your view?
hi @steveWinter
Thanks for your contribution, very appreciated!
IMHO this is a legit solution and easy to remove once SF 3.4 is not supported anymore.
Any updates?
Not yet from my side. I'm still trying to fix the test suite. Maybe @steveWinter progressed better?
I got the test suite (mostly) sorted see https://github.com/steveWinter/OneupUploaderBundle and then started working on SF5 updates, but then life and work got in the way and I've not had a chance to get back to it yet.
but then life and work got in the way and I've not had a chance to get back to it yet
Same here 😁
It would possibly make it way more easy, if we ditch the support for Symfony 3.4. What do you think?
In my opinion, symfony 3.4 should no longer be supported in newer versions of bundle.
Those who use it should use the bundle version that support 3.4
We should focus on Symfony 5 and how to fix tests.
From what I've seen, I think the tests should be refactored
Especially the part where the test client is created
I'm ok with dropping 3.4 - and in fact based on where I got up to I don't think it's possible to support 3.4 and 5.0 in the same version anyway (the event system in 5.0 relies on interfaces which aren't in 3.4).
And @stipic you're correct that the instantiation of the test client is also significantly different for 5 - I've resolved that, but again, it's incompatible with 3.4...
So... @bytehead this is your bundle - if you're happy to drop 3.4 then I can try and come back to getting this up to 5.0 this coming Friday...
@steveWinter I'm fine with dropping 3.4!
@steveWinter sorry for the annoyance
can you at least push the last update to your forked repo
We can help. You came the farthest
This fork: https://github.com/steveWinter/OneupUploaderBundle has the latest of what I've been able to do. Tests pass for SF 4.4 on PHP 7.2, but fail on SF5.0 due to interface changes (at least).
@steveWinter - You probably also need to update Twig/Extension/UploaderExtension.php for Twig 3. The existing \Twig_Extension and \Twig_SimpleFunction need to be swapped out with Twig\Extension\AbstractExtension and Twig\TwigFunction.
I think the composer.json file also needs to declare which version of Twig is required due to the Twig Twig extension.
I'm not sure which point release of Twig 2 the TwigFunction and AbstractExtension classes were introduced but looking at the existing Twig extensions (html-extra, inky-extra, etc.), it looks like you could go with "twig/twig": "^2.4|^3.0".
And I realized I shouldn't be lazy and did the changes myself and sent @steveWinter a pull request to his repo.
I've pulled all changes of you @steveWinter and @gubler over to #373. There's a shitload to refactor but I have to release a new major version anyway.
I cant install this feature branch 😢
$ composer require oneup/uploader-bundle:dev-feature-symfony-5
$ composer require oneup/uploader-bundle:dev-feature/symfony-5
Can you change the name maybe to simple symfony-5 like other repositories do?
Thanks in Advance 🎉
It work's in my project:
...
"require": {
"php": "^7.1.3",
"ext-ctype": "*",
"ext-iconv": "*",
"oneup/uploader-bundle": "dev-feature/symfony-5",
"symfony/console": "4.4.*",
"symfony/dotenv": "4.4.*",
"symfony/flex": "^1.3.1",
"symfony/framework-bundle": "4.4.*",
"symfony/yaml": "4.4.*"
},
...
Whats the composer error message?
It goes well for me BUT symfony5 is dev branch and bundle doesn't work for me yet i tried yesteraday and my Events are not triggered on upload so i decided to wait.
Yes the events are somehow broken, thats why tests still fail.
I've you have a clue about it, hint are welcome 😉
Whats the composer error message?
Could not find package oneup/uploader-bundle in a version matching dev-feature-symfony-5
But with your composer.json its working.
I think the Composer CLI cant handle the slash 😅
I've you have a clue about it, hint are welcome 😉
I'll fix it if I'm able to do 😅
I've you have a clue about it, hint are welcome 😉
Its very very very interesting this behaviour there.
I debuged now round about 6 hours. (really! No joke 😅)
Its so interesting why this isnt working.
Every time I think I found the problem but nope, not.
The single upload \Oneup\UploaderBundle\Tests\Controller\AbstractUploadTest::testSingleUpload works fine.
Even when I clone the
$client->request('POST', $endpoint, $this->getRequestParameters(), [$this->getRequestFile()], $this->requestHeaders);
Line and have it multiple times.
The dispatcher will be called multiple times.
Good...
So testing is working fine.
But in the AbstractChunkedUploadTest there is something which will destroy the logic.
Because it seems that every time you call an $client->request you'll get a completly new event dispatcher.
When you set protected $total = 1 then its working fine, because the Chunk is only one and its more similar to the Single-Upload.
But with 6 its uploading all 5 chunks and with the sixt one you'll get the ValidationEvent called.
And... WTF.... the Dispatcher dont see this ValidationEvent. I cant find out why.
I only checked the situation and find out that its working - the event is getting called.
And thats it.
So @stipic:
It goes well for me BUT symfony5 is dev branch and bundle doesn't work for me yet i tried yesteraday and my Events are not triggered on upload so i decided to wait.
I think you're using the wrong events.
The new dispatcher requries "cool event names" but at the moment this bundle uses the class of the event as name.
For example you have a const in \Oneup\UploaderBundle\Event\ValidationEvent called NAME.
And this is pointing to oneup_uploader.validation which is never used.
The event which you must listen to is \Oneup\UploaderBundle\Event\ValidationEvent::class.
This event will be called and this is working.
I'll continue with using this branch of the bundle but I stop now the investigation why the client have a weired behavior.
Very interesting story. Maybe someone else want to spend more time and find a solution 👍
Because it seems that every time you call an $client->request you'll get a completly new event dispatcher.
Exactly where I ended up 😅 At least we have consistency here.
Because it seems that every time you call an $client->request you'll get a completly new event dispatcher.
Actually not even everytime:
While running testChunkedUpload() I've dumped the IDs of the event dispatcher object (with spl_object_id()):
^ 1829
^ 1829
^ 270
^ 169
^ 424
^ 5260
This makes totally no sense in any way to me... 🤔🤯
@patrickbussmann I had to disable rebooting the kernel when setting up the client: https://github.com/symfony/framework-bundle/blob/98eb08495421dec27077c58b70e96f52be49b66d/KernelBrowser.php#L88-L94
The client shuts down by default after a request...
But @bytehead thats what I mean with it makes absolutly no sense.
Because I had the same thoughts and think it could be something kernel related or this.
But take a look at a simple function.
\Oneup\UploaderBundle\Tests\Controller\AbstractUploadTest::testEvents
We have a listener for PostUploadEvent and one for PreUploadEvent.
One of them counting ++$uploadCount;.
So now lets run the test.
$ vendor\bin\phpunit --filter testEvents
Successful!
Testing OneupUploaderBundle test suite
.............. 14 / 14 (100%)
Time: 749 ms, Memory: 20.00 MB
OK (14 tests, 70 assertions)
So now lets try what happen when we test your sentence.
The client shuts down by default after a request...
So I copy this line 4 times. (you can do it as often as you like)
$client->request('POST', $endpoint, $this->getRequestParameters(), [$this->getRequestFile()]);
So and now lets run the test again.
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.
Testing OneupUploaderBundle test suite
..F.F.....F.F. 14 / 14 (100%)
Time: 879 ms, Memory: 22.00 MB
**There were 4 failures:**
1) Oneup\UploaderBundle\Tests\Controller\DropzoneTest::testEvents
Failed asserting that actual size 4 matches expected size 1.
2) Oneup\UploaderBundle\Tests\Controller\FancyUploadTest::testEvents
Failed asserting that actual size 4 matches expected size 1.
3) Oneup\UploaderBundle\Tests\Controller\UploadifyTest::testEvents
Failed asserting that actual size 4 matches expected size 1.
4) Oneup\UploaderBundle\Tests\Controller\YUI3Test::testEvents
Failed asserting that actual size 4 matches expected size 1.
FAILURES!
Tests: 14, Assertions: 62, Failures: 4.
So lets think about.
When the kernel is destroyed and our dispatcher will be deleted or recreated or or or...
Why the event was called successfully 4 times? - as expected?
I mean the problem is that when we do the same in the AbstractChunkedUploadTest its not working 🤣
And thats why I cant understand this problem.
Its working fine in AbstractUploadTest but not in AbstractChunkedUploadTest.
😑
P.S.: In the Screenshot I already moved to ::NAME instead ::class which I will upload in pull request soon.
Interesting that it works with your solution.
I renamed the event names so that its as described in the Symfony 5 docs:
#374
Thanks for solving our issue where we spent multiple hours on it with one line code 😅👍
Is feature/symfony-5 safe for use now or?
Would you mind to give it a try? I didn't have the time to test it so far in a real world example... 🙈
@stipic I already use it in a productive project which is fully automated tested.
And until now there are no issues with it.
I'm using local upload.
Thank you for feedbacks. I will use it then in production. After few days i will give feedback :)
Everything OK for me after 2 weeks in very active production.
Thanks.
Thank you for testing and the feedback! Will merge soon and release a new version.