I've refactored the makePathRelative function #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can you open the Pull Request on symfony/symfony as this repository is read-only? Thanks a lot. |
|
Closing as it has now been submitted on symfony/symfony. |
fabpot
added a commit
that referenced
this pull request
May 16, 2013
Commits ------- eae6a77 fixed wrong case d0a175b fixes #1659 f300ede fixes several bugs a4f05ac added some tests Discussion ---------- Http util fixes Fixes several bugs in the http utils. Please don't add anymore features without sufficient tests. Especially for the Security\Http namespace, regressions are very likely otherwise. --------------------------------------------------------------------------- by fabpot at 2011/07/19 22:37:26 -0700 Tests do not pass for me: There were 2 errors: 1) Symfony\Bundle\SecurityBundle\Tests\Functional\LocalizedRoutesAsPathTest::testLoginLogoutProcedure with data set #0 ('en') InvalidArgumentException: The current node list is empty. .../src/Symfony/Component/DomCrawler/Crawler.php:604 .../src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php:16 2) Symfony\Bundle\SecurityBundle\Tests\Functional\LocalizedRoutesAsPathTest::testLoginLogoutProcedure with data set #1 ('de') InvalidArgumentException: The current node list is empty. .../src/Symfony/Component/DomCrawler/Crawler.php:604 .../src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php:16 -- There were 4 failures: 1) Symfony\Bundle\SecurityBundle\Tests\Functional\LocalizedRoutesAsPathTest::testAccessRestrictedResource with data set #0 ('en') Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -http://localhost/en/login +http://localhost/login .../src/Symfony/Bundle/Securitybundle/Tests/Functional/WebTestCase.php:22 .../src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php:38 2) Symfony\Bundle\SecurityBundle\Tests\Functional\LocalizedRoutesAsPathTest::testAccessRestrictedResource with data set #1 ('de') Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -http://localhost/de/login +http://localhost/login .../src/Symfony/Bundle/Securitybundle/Tests/Functional/WebTestCase.php:22 .../src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php:38 3) Symfony\Bundle\SecurityBundle\Tests\Functional\LocalizedRoutesAsPathTest::testAccessRestrictedResourceWithForward with data set #0 ('en') HTTP/1.0 302 Found Cache-Control: no-cache Content-Length: 299 Content-Type: text/html; charset=UTF-8 Date: Wed, 20 Jul 2011 05:36:27 GMT Location: http://localhost/login Set-Cookie: PHPSESSID=11c9c6a7e7620e13bddef223a5ba46d9; path=/; domain= <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> <meta http-equiv="refresh" content="1;url=http://localhost/login" /> </head> <body> Redirecting to <a href="http://localhost/login">http://localhost/login</a>. </body> </html> Failed asserting that <integer:0> matches expected <integer:1>. .../src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php:50 4) Symfony\Bundle\SecurityBundle\Tests\Functional\LocalizedRoutesAsPathTest::testAccessRestrictedResourceWithForward with data set #1 ('de') HTTP/1.0 302 Found Cache-Control: no-cache Content-Length: 299 Content-Type: text/html; charset=UTF-8 Date: Wed, 20 Jul 2011 05:36:28 GMT Location: http://localhost/login Set-Cookie: PHPSESSID=2bbe63786a088471ade3717917f4ba4f; path=/; domain= <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> <meta http-equiv="refresh" content="1;url=http://localhost/login" /> </head> <body> Redirecting to <a href="http://localhost/login">http://localhost/login</a>. </body> </html> Failed asserting that <integer:0> matches expected <integer:1>. .../src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php:50 --------------------------------------------------------------------------- by schmittjoh at 2011/07/19 23:47:29 -0700 I fixed a wrong case, but I couldn't reproduce the other errors (tested on Ubuntu). My guess is that the temporary directory on your machine couldn't be deleted for some reason, and the test runs with the configuration of some of the previous tests. --------------------------------------------------------------------------- by fabpot at 2011/07/20 00:28:41 -0700 That does not make any difference for me. For instance, in `LocalizedRoutesAsPathTest::testLoginLogoutProcedure()`, the first request to `'/'.$locale.'/login'` returns the following Response: <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> <meta http-equiv="refresh" content="1;url=http://localhost/login" /> </head> <body> Redirecting to <a href="http://localhost/login">http://localhost/login</a>. </body> </html> --------------------------------------------------------------------------- by schmittjoh at 2011/07/20 00:31:34 -0700 That's weird, did you make sure that the temporary directory does not exist? ``rm -Rf /tmp/StandardFormLogin/`` On Wed, Jul 20, 2011 at 9:28 AM, fabpot < reply@reply.github.com>wrote: > That does not make any difference for me. For instance, in > `LocalizedRoutesAsPathTest::testLoginLogoutProcedure()`, the first request > to `'/'.$locale.'/login'` returns the following Response: > > <html> > <head> > <meta http-equiv="Content-Type" content="text/html; > charset=utf-8" /> > <meta http-equiv="refresh" content="1;url= > http://localhost/login" /> > </head> > <body> > Redirecting to <a href="http://localhost/login"> > http://localhost/login</a>. > </body> > </html> > > -- > Reply to this email directly or view it on GitHub: > symfony/symfony#1739 (comment) > --------------------------------------------------------------------------- by fabpot at 2011/07/20 00:33:40 -0700 Yes, I've just checked and the directory does not exist. --------------------------------------------------------------------------- by schmittjoh at 2011/07/20 00:39:55 -0700 Sorry, I can't reproduce it on Ubuntu and unless someone wants to sponsor me a Mac, there is not much I can do.
fabpot
added a commit
that referenced
this pull request
May 16, 2013
Commits ------- 07298ac Detect EOF when reading input stream Discussion ---------- [Console] Detect EOF when reading input stream This is related to commits 511a9a1 and 3a5d508. First of them introduced abort-on-EOF and the second regressed the functionality. Problem is stream_get_line() doesn't return false on EOF. So it needs call to feof() to detect the situation. Still, it's not ideal. With fgets() it worked fine, but with stream_get_line() one has to press CTRL+D twice to get out. I presume this could be bug in PHP itself. But better than nothing. Please consider. --------------------------------------------------------------------------- by fabpot at 2011/07/19 22:47:53 -0700 I have used `stream_get_line` especially because it does not return `false` on eof. This is needed when you pass your own stream for unit tests. --------------------------------------------------------------------------- by lenar at 2011/07/25 06:05:59 -0700 This is not the best solution I think. Tests should mimic and cope with real life not the other way around. Better solution would be to fix testcase. Like this: lenar/SensioGeneratorBundle@6ff3f26. Or maybe create a special "testing" stream wrapper that wraps php://memory and gives out just linefeeds after real data ends. And then change stream_get_line() back to fgets() if there is no other reason for this change. --------------------------------------------------------------------------- by fabpot at 2011/07/25 06:24:20 -0700 When applying your patch to the generator bundle (and revert to use `fgets`), I get "RuntimeException: Aborted" exceptions. --------------------------------------------------------------------------- by lenar at 2011/07/25 06:35:08 -0700 With d326f89 added + lenar/SensioGeneratorBundle@6ff3f26 I can successfully run every test in that file. --------------------------------------------------------------------------- by fabpot at 2011/07/26 23:31:36 -0700 @lenar: not for me. I have many 'Aborted' exception on my Mac. --------------------------------------------------------------------------- by fabpot at 2011/07/26 23:41:18 -0700 And I have the exact same errors on Linux: There were 7 errors: 1) Sensio\Bundle\GeneratorBundle\Tests\Command\GenerateBundleCommandTest::testInteractiveCommand with data set #0 (array('/tmp'), 'Foo/BarBundle ', array('Foo\\BarBundle', 'FooBarBundle', '/tmp/', 'annotation', false)) RuntimeException: Aborted .../Symfony/Component/Console/Helper/DialogHelper.php:40 .../Symfony/Component/Console/Helper/DialogHelper.php:97 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Command/GenerateBundleCommand.php:165 .../Symfony/Component/Console/Command/Command.php:205 .../Symfony/Component/Console/Tester/CommandTester.php:66 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Tests/Command/GenerateBundleCommandTest.php:39 2) Sensio\Bundle\GeneratorBundle\Tests\Command\GenerateBundleCommandTest::testInteractiveCommand with data set #1 (array('/tmp'), 'Foo/BarBundle BarBundle foo yml n', array('Foo\\BarBundle', 'BarBundle', 'foo/', 'yml', false)) RuntimeException: Aborted .../Symfony/Component/Console/Helper/DialogHelper.php:40 .../Symfony/Component/Console/Helper/DialogHelper.php:62 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Command/GenerateBundleCommand.php:83 .../Symfony/Component/Console/Command/Command.php:214 .../Symfony/Component/Console/Tester/CommandTester.php:66 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Tests/Command/GenerateBundleCommandTest.php:39 3) Sensio\Bundle\GeneratorBundle\Tests\Command\GenerateBundleCommandTest::testInteractiveCommand with data set #2 (array('/tmp', 'yml', 'BarBundle', true), 'Foo/BarBundle ', array('Foo\\BarBundle', 'BarBundle', '/tmp/', 'yml', true)) RuntimeException: Aborted .../Symfony/Component/Console/Helper/DialogHelper.php:40 .../Symfony/Component/Console/Helper/DialogHelper.php:97 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Command/GenerateBundleCommand.php:165 .../Symfony/Component/Console/Command/Command.php:205 .../Symfony/Component/Console/Tester/CommandTester.php:66 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Tests/Command/GenerateBundleCommandTest.php:39 4) Sensio\Bundle\GeneratorBundle\Tests\Command\GenerateDoctrineEntityCommandTest::testInteractiveCommand with data set #0 (array(), 'AcmeBlogBundle:Blog/Post ', array('Blog\\Post', 'annotation', array())) RuntimeException: Aborted .../Symfony/Component/Console/Helper/DialogHelper.php:40 .../Symfony/Component/Console/Helper/DialogHelper.php:97 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Command/GenerateDoctrineEntityCommand.php:145 .../Symfony/Component/Console/Command/Command.php:205 .../Symfony/Component/Console/Tester/CommandTester.php:66 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Tests/Command/GenerateDoctrineEntityCommandTest.php:39 5) Sensio\Bundle\GeneratorBundle\Tests\Command\GenerateDoctrineEntityCommandTest::testInteractiveCommand with data set #1 (array('AcmeBlogBundle:Blog/Post'), '', array('Blog\\Post', 'annotation', array())) RuntimeException: Aborted .../Symfony/Component/Console/Helper/DialogHelper.php:40 .../Symfony/Component/Console/Helper/DialogHelper.php:97 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Command/GenerateDoctrineEntityCommand.php:121 .../Symfony/Component/Console/Command/Command.php:205 .../Symfony/Component/Console/Tester/CommandTester.php:66 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Tests/Command/GenerateDoctrineEntityCommandTest.php:39 6) Sensio\Bundle\GeneratorBundle\Tests\Command\GenerateDoctrineEntityCommandTest::testInteractiveCommand with data set #2 (array(), 'AcmeBlogBundle:Blog/Post yml ', array('Blog\\Post', 'yml', array())) RuntimeException: Aborted .../Symfony/Component/Console/Helper/DialogHelper.php:40 .../Symfony/Component/Console/Helper/DialogHelper.php:62 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Command/GenerateDoctrineEntityCommand.php:153 .../Symfony/Component/Console/Command/Command.php:205 .../Symfony/Component/Console/Tester/CommandTester.php:66 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Tests/Command/GenerateDoctrineEntityCommandTest.php:39 7) Sensio\Bundle\GeneratorBundle\Tests\Command\GenerateDoctrineEntityCommandTest::testInteractiveCommand with data set #3 (array(), 'AcmeBlogBundle:Blog/Post yml title 255 description text ', array('Blog\\Post', 'yml', array(array('title', 'string', 255), array('description', 'text')))) RuntimeException: Aborted .../Symfony/Component/Console/Helper/DialogHelper.php:40 .../Symfony/Component/Console/Helper/DialogHelper.php:62 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Command/GenerateDoctrineEntityCommand.php:153 .../Symfony/Component/Console/Command/Command.php:205 .../Symfony/Component/Console/Tester/CommandTester.php:66 .../symfony-standard/vendor/bundles/Sensio/Bundle/GeneratorBundle/Tests/Command/GenerateDoctrineEntityCommandTest.php:39 --------------------------------------------------------------------------- by lenar at 2011/07/26 23:56:46 -0700 @fabpot: and you modified all those tests? I only modified ```Tests/Command/GenerateDoctrineCrudCommandTest.php``` and that doesn't fail as I see from your log. I just provided example, though I could add necessary changes for other tests too. --------------------------------------------------------------------------- by fabpot at 2011/07/27 00:09:32 -0700 @lenar: ah, sorry about that. Then, can you provide a fix for all the other tests too? Thanks a lot. --------------------------------------------------------------------------- by lenar at 2011/07/27 00:22:54 -0700 @fabpot: actually what do you think about this kind of fix instead for tests: lenar/SensioGeneratorBundle@517f263cb01ea2ea1ef2 instead my previous proposal (lenar/SensioGeneratorBundle@6ff3f26). Really simple, short and effective. --------------------------------------------------------------------------- by fabpot at 2011/07/27 00:37:51 -0700 @lenar: looks good to me. Can you create a PR? --------------------------------------------------------------------------- by lenar at 2011/07/27 00:45:36 -0700 @fabpot: sensiolabs/SensioGeneratorBundle#60
fabpot
pushed a commit
that referenced
this pull request
May 16, 2013
[HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic (without the BC break)
fabpot
added a commit
that referenced
this pull request
May 16, 2013
Commits ------- 7c2f11f Merge pull request #1 from pminnieur/post_response 9f4391f [HttpKernel] fixed DocBlocks 2a61714 [HttpKernel] added PostResponseEvent dispatching to HttpKernel 915f440 [HttpKernel] removed BC breaks, introduced new TerminableInterface 7efe4bc [HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic Discussion ---------- [HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic This came out of a discussion on IRC about doing stuff post-response, and the fact that right now there is no best practice, and it basically requires adding code after the `->send()` call. It's an attempt at fixing it in an official way. Of course terminate() would need to be called explicitly, and added to the front controllers, but then it offers a standard way for everyone to listen on that event and do things without slowing down the user response. --------------------------------------------------------------------------- by stof at 2011/12/06 02:41:26 -0800 We discussed it on IRC and I suggested a way to avoid the BC break of the interface: adding a new interface (``TerminableInterface`` or whatever better name you find) containing this method. HttpKernel, Kernel and HttpCache can then implement it without breaking the existing apps using the component (Kernel and HttpCache would need an instanceof check to see if the inner kernel implements the method) For Symfony2 users it will mean they have to change their front controller to benefit from the new event of course, but this is easy to do. Btw, Silex can then be able to use it without *any* change for the end users as it can be done inside ``Application::run()`` --------------------------------------------------------------------------- by pminnieur at 2011/12/06 11:47:03 -0800 @Seldaek: I opened a pull request so that the discussion on IRC is fulfilled and no BC breaks exist: https://github.com/Seldaek/symfony/pull/1/files --------------------------------------------------------------------------- by fabpot at 2011/12/07 07:59:49 -0800 Any real-world use case for this? --------------------------------------------------------------------------- by Seldaek at 2011/12/07 08:10:31 -0800 Doing slow stuff after the user got his response back without having to implement a message queue. I believe @pminnieur wanted to use it to send logs to loggly? --------------------------------------------------------------------------- by pminnieur at 2011/12/07 09:08:41 -0800 Its a good practice to defer code execution without the introduction of a new software layer (like gearman, amqp, whatever tools people use to defer code execution) which may be way too much just for the goal of having fast responses, whatever my code does. My real world use case which made me miss this feature the first time: > I have a calendar with a scheduled Event. For a given period of time, several Event entities will be created, coupled to the scheduled event (the schedule Event just keeps track of `startDate`, `endDate` and the `dateInterval`). Let's say we want this scheduled Event to be on every Monday-Friday, on a weekly basis, for the next 10 years. This means I have to create `10*52*5` Event entities before I could even think about sending a simple redirect response. If I could defer code execution, I'd only save the scheduled Event, send the redirect response and after that, I create the `10*52*5` entities. The other use case was loggly, yes. Sending logging data over the wire before the response is send doesn't make sense in my eyes, so it could be deferred after the response is send (this especially sucks if loggly fails and i get a 500 --the frontend/public user is not interested in a working logging facility, he wants his responses). --------------------------------------------------------------------------- by mvrhov at 2011/12/07 10:07:03 -0800 This would help significantly, but the real problem, that your process is busy and unavailable for the next request, is still there. --------------------------------------------------------------------------- by fabpot at 2011/12/07 10:15:18 -0800 I think this is the wrong solution for a real problem. Saying "Its a good practice to defer code execution without the introduction of a new software layer" is just wrong. It is definitely a good practice to defer code execution, but you should use the right tool for the job. I'm -1. --------------------------------------------------------------------------- by pminnieur at 2011/12/07 10:25:44 -0800 It should just give a possibility to put unimportant but heavy lifting code behind the send request with ease. With little effort people could benefit from the usage of `fastcgi_finish_request` without introducing new software, using `register_shutdown_function` or using `__destruct `(which works for simple things, but may act weird with dependencies). It should not simulate node.js ;-) I agree that the real problem is not solved, but small problems could be solved easily. I personally don't want to setup RabbitMQ or whatever, maintain my crontab or any other software that may allow me to defer code execution. --------------------------------------------------------------------------- by Seldaek at 2011/12/08 01:08:32 -0800 @fabpot: one could say that on shared hostings it is still useful because they generally don't give you gearman or \*MQs. Anyway I think it'd be nice to really complete the HttpKernel event cycle. --------------------------------------------------------------------------- by pminnieur at 2011/12/08 01:48:57 -0800 not only on shared hostings, sometimes teams/projects just don't have the resources or knowledge or time to setup such an infrastructure. --------------------------------------------------------------------------- by videlalvaro at 2011/12/08 01:53:06 -0800 I can say we used `fastcgi_finish_request` quite a lot at poppen with symfony 1.x. It certainly helped us to send data to Graphite, save XHProf runs, send data to RabbitMQ, and so on. For example we used to connect to RabbitMQ and send the messages _after_ calling `fastcgi_finish_request` so the user never had to wait for stuff like that. Also keep in mind that if you are using Gearman or RabbitMQ or whatever tool you use to defer code execution… you are not deferring the network connection handling, sending data over the wire and what not. I know this is obvious but is often overlooked. So it would be nice to have an standard way of doing this. --------------------------------------------------------------------------- by henrikbjorn at 2011/12/13 01:42:23 -0800 This could have been useful recently while implementing a "Poor mans cronjob" system. The solution was to do a custom Response object and do the stuff after send have been called with a Connection: Close header and ignore_user_abort(); (Yes very ugly)
fabpot
added a commit
that referenced
this pull request
May 16, 2013
Commits ------- 753c067 [FrameworkBundle] added $view['form']->csrfToken() helper e1aced8 [Twig] added {{ csrf_token() }} helper Discussion ---------- [Twig] [FrameworkBundle] added CSRF token helper I've added a templating helper and Twig function for generating a CSRF token without the overhead of creating a form. ```html+jinja <form action="{{ path('user_delete', { 'id': user.id }) }}" method="post"> <input type="hidden" name="_method" value="delete"> <input type="hidden" name="_token" value="{{ csrf_token('delete_user_' ~ user.id) }}"> <button type="submit">delete</button> </form> ``` ```php <?php class UserController extends Controller { public function delete(User $user, Request $request) { $csrfProvider = $this->get('form.csrf_provider'); if (!$csrfProvider->isCsrfTokenValid('delete_user_'.$user->getId(), $request->request->get('_token')) { throw new RuntimeException('CSRF attack detected.'); } // etc... } } ``` The test that is failing on Travis appears to be unrelated, but I may be wrong? ``` 1) Symfony\Bundle\SecurityBundle\Tests\Functional\LocalizedRoutesAsPathTest::testLoginLogoutProcedure with data set #1 ('de') RuntimeException: OUTPUT: Catchable fatal error: Argument 3 passed to Symfony\Bundle\FrameworkBundle\Controller\TraceableControllerResolver::__construct() must be an instance of Symfony\Component\HttpKernel\Debug\Stopwatch, instance of Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser given, called in /tmp/2.1.0-DEV/StandardFormLogin/cache/securitybundletest/appSecuritybundletestDebugProjectContainer.php on line 94 and defined in /home/vagrant/builds/kriswallsmith/symfony/src/Symfony/Bundle/FrameworkBundle/Controller/TraceableControllerResolver.php on line 37 ``` --------------------------------------------------------------------------- by pablodip at 2012-01-10T14:18:45Z As you don't need forms to use the csrf provider, how about putting its service without the form prefix? It could even make sense to put the CsrfProvider as a component since you can use it standalone and in more cases than only forms. It would be a small component though. --------------------------------------------------------------------------- by Tobion at 2012-01-10T17:54:14Z I think it would be more clear to generate the token in the controller. Doing so in the template will spread the CSRF intention across template and controller. So I don't think this extension is necessary. --------------------------------------------------------------------------- by kriswallsmith at 2012-01-10T17:58:14Z @pablodip I'm open to the idea of a Csrf component. This would be a good place for some nonce classes as well. @Tobion I disagree. One use case is for a list of users, each with a delete form. Iterating over the users in the controller and generating a token for each, just to iterate over them again in the view is a waste and adds complexity. --------------------------------------------------------------------------- by Tobion at 2012-01-10T18:05:14Z I see. But I don't understand why the intention needs to be different for each user to delete. Usually the intention is the same for each form type. I thought this is enough. --------------------------------------------------------------------------- by kriswallsmith at 2012-01-10T18:06:13Z Yes, a static intention would suffice. --------------------------------------------------------------------------- by Tobion at 2012-01-10T18:07:08Z Then your use case is not valid anymore. --------------------------------------------------------------------------- by Tobion at 2012-01-10T18:12:25Z I would suggest to make a cookbook article out of it about how to create a simple form without the form component. And include such things as validating the result using the validator component and checking the CSRF. --------------------------------------------------------------------------- by kriswallsmith at 2012-01-10T21:32:50Z This helper makes it easier to use CSRF protection without a form and we should make it as easy as possible. Spreading the intention across controller and template is not concerning to me. Either way, a cookbook entry is a great idea. --------------------------------------------------------------------------- by Tobion at 2012-01-10T21:47:12Z Well, it's just one line more without this helper. So I disagree it makes it really easier when you know how to use the CsrfProvider which is a pre-condition anyway since you must still validate its correctness by hand. --------------------------------------------------------------------------- by kriswallsmith at 2012-01-13T13:24:15Z Another use case is when rendering a page with a bunch of simple buttons with different intentions: delete user, delete comment, follow, unfollow... Creating all of these in the controller just leads to spaghetti. --------------------------------------------------------------------------- by jwage at 2012-01-17T21:55:53Z :+1: lots of use cases for something like this @opensky
fabpot
added a commit
that referenced
this pull request
May 16, 2013
Commits ------- 100e97e [Filesystem] Fixed warnings in makePathRelative(). f5f5c21 [Filesystem] Fixed typos in the docblocks. d4243a2 [Filesystem] Fixed a bug in remove being unable to remove symlinks to unexisting file or directory. 11a676d [Filesystem] Added unit tests for mirror method. 8c94069 [Filesystem] Added unit tests for isAbsolutePath method. 2ee4b88 [Filesystem] Added unit tests for makePathRelative method. 21860cb [Filesystem] Added unit tests for symlink method. a041feb [Filesystem] Added unit tests for rename method. 8071859 [Filesystem] Added unit tests for chmod method. bba0080 [Filesystem] Added unit tests for remove method. 8e861b7 [Filesystem] Introduced workspace directory to limit complexity of tests. a91e200 [Filesystem] Added unit tests for touch method. 7e297db [Filesystem] Added unit tests for mkdir method. 6ac5486 [Filesystem] Added unit tests for copy method. 1c833e7 [Filesystem] Added missing docblock comment. Discussion ---------- [Filesystem] Fixed a bug in remove() being unable to unlink broken symlinks While working on test coverage for Filesystem class I discovered a bug in remove() method. Before removing a file a check is made if it exists: if (!file_exists($file)) { continue; } Problem is [file_exists()](http://php.net/file_exists) returns false if link's target file doesn't exist. Therefore remove() will fail to delete a directory containing a broken link. Solution is to handle links a bit different: if (!file_exists($file) && !is_link($file)) { continue; } Additionally, this PR improves test coverage of Filesystem component. Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes --------------------------------------------------------------------------- by cordoval at 2012-04-07T00:55:59Z ✌.|•͡˘‿•͡˘|.✌ --------------------------------------------------------------------------- by fabpot at 2012-04-07T06:12:34Z Tests do not pass for me: PHPUnit 3.6.10 by Sebastian Bergmann. Configuration read from /Users/fabien/work/symfony/git/symfony/phpunit.xml.dist .........................EE....... Time: 0 seconds, Memory: 5.25Mb There were 2 errors: 1) Symfony\Component\Filesystem\Tests\FilesystemTest::testMakePathRelative with data set #0 ('/var/lib/symfony/src/Symfony/', '/var/lib/symfony/src/Symfony/Component', '../') Uninitialized string offset: 29 .../rc/Symfony/Component/Filesystem/Filesystem.php:183 .../rc/Symfony/Component/Filesystem/Tests/FilesystemTest.php:434 2) Symfony\Component\Filesystem\Tests\FilesystemTest::testMakePathRelative with data set #1 ('var/lib/symfony/', 'var/lib/symfony/src/Symfony/Component', '../../../') Uninitialized string offset: 16 .../rc/Symfony/Component/Filesystem/Filesystem.php:183 .../rc/Symfony/Component/Filesystem/Tests/FilesystemTest.php:434 FAILURES! Tests: 34, Assertions: 67, Errors: 2. --------------------------------------------------------------------------- by jakzal at 2012-04-07T07:26:15Z Sorry for this. For some reason my PHP error reporting level was to low to catch this... Should be fixed now but I needed to modify the makePathRelative() (this bug existed before).
fabpot
pushed a commit
that referenced
this pull request
May 16, 2013
[FileSystem] explain possible failure of symlink creation in windows
fabpot
added a commit
that referenced
this pull request
May 16, 2013
Commits ------- a20fc68 Merge pull request #1 from SamsonIT/FilesystemExceptions 8eca661 [FileSystem] explains possible failure of symlink creation in windows b1f8744 Add Changelog BC Break note 24eb396 [Filesystem] Added few new behaviors: Discussion ---------- [Filesystem] Consistence and enhancements for Filesystem Bug fix: no Feature addition: yes Backwards compatibility break: **yes** Symfony2 tests pass: yes Fixes the following tickets: None License of the code: MIT This PR adds features and introduce a backward compatibility break. features : - whenever an action fails, a \RuntimeException is thrown - add access to the second and third arguments of ``touch`` function - add a recursive option for chmod - add a chown method - add a chgrp method The backward compatibility break happens in the mkdir method : Before this PR, a boolean is returned ; true if all directories were created, false otherwise. It now returns nothing. --------------------------------------------------------------------------- by travisbot at 2012-05-18T14:26:42Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1367000) (merged 83cdd622 into 1e15f21). --------------------------------------------------------------------------- by fabpot at 2012-05-20T02:40:28Z To be consistent, we should throw exception whenever some operation fails. --------------------------------------------------------------------------- by romainneutron at 2012-05-20T21:10:23Z I fix the consistency ; mkdir now throws an exception if a directory creation fails. This introduce a BC break, see PR message which has been updated with all features and BC break. Added chgrp and chown methods Add options for touch Add recursive option for chmod --------------------------------------------------------------------------- by travisbot at 2012-05-20T21:11:47Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1383619) (merged a4d1eeb8 into 1407f11). --------------------------------------------------------------------------- by travisbot at 2012-05-22T10:49:06Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1399027) (merged 7e14b6bd into 517ae43). --------------------------------------------------------------------------- by travisbot at 2012-05-22T10:58:10Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1399083) (merged 71852653 into 517ae43). --------------------------------------------------------------------------- by travisbot at 2012-05-22T11:18:44Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1399194) (merged 7645bad3 into 517ae43). --------------------------------------------------------------------------- by travisbot at 2012-05-23T18:21:47Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1414091) (merged b049d5b1 into 517ae43). --------------------------------------------------------------------------- by travisbot at 2012-05-23T18:26:19Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1414123) (merged 34903466 into 517ae43). --------------------------------------------------------------------------- by travisbot at 2012-05-29T16:07:26Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1467173) (merged b1d1eb2e into adf07f1). --------------------------------------------------------------------------- by travisbot at 2012-05-29T16:19:38Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1467261) (merged 42015ffa into adf07f1). --------------------------------------------------------------------------- by romainneutron at 2012-06-01T14:30:45Z Any news about this PR ? --------------------------------------------------------------------------- by stloyd at 2012-06-08T09:57:39Z @romainneutron You need to [squash](http://www.silverwareconsulting.com/index.cfm/2010/6/6/Using-Git-Rebase-to-Squash-Commits) your commits, and add more proper message in squashed commit i.e.: > [Filesystem] Added few new behaviors: * whenever an action fails, a `RuntimeException` is thrown * add access to the second and third arguments of `touch()` function * add a recursive option for `chmod()` * add a `chown()` method * add a `chgrp()` method > BC break: `mkdir()` function throw exception in case of failture instead of returning Boolean value. --------------------------------------------------------------------------- by romainneutron at 2012-06-08T10:59:55Z @stloyd squash done ! --------------------------------------------------------------------------- by travisbot at 2012-06-08T11:26:20Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1565540) (merged 8f55ddb6 into f8e68e5). --------------------------------------------------------------------------- by travisbot at 2012-06-08T11:41:45Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1566247) (merged 880312b6 into f8e68e5). --------------------------------------------------------------------------- by romainneutron at 2012-06-09T11:42:24Z I've added documentation to the Filesystem component : symfony/symfony-docs#1439 --------------------------------------------------------------------------- by travisbot at 2012-06-09T16:47:20Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1577754) (merged 5647ad41 into f8a09db). --------------------------------------------------------------------------- by stloyd at 2012-06-13T14:47:31Z @romainneutron You probably need to rebase your code as some changes were merge into master for `Filesystem`. --------------------------------------------------------------------------- by romainneutron at 2012-06-13T15:17:31Z @stloyd rebase OK ! by the way, do you have any idea when/if this PR will be merged ? --------------------------------------------------------------------------- by travisbot at 2012-06-13T15:20:44Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1611591) (merged c8b86c68 into c07e916). --------------------------------------------------------------------------- by fabpot at 2012-06-16T16:40:50Z You need to add a note about the BC breaks in the CHANGELOG file. --------------------------------------------------------------------------- by fabpot at 2012-06-16T16:43:20Z Also, instead of using `\RuntimeException`, I would create a custom exception like we have done in other components (an interface + a RuntimeException that implements the interface and extends \RuntimeException). The exception name can be something like `IOException`. --------------------------------------------------------------------------- by travisbot at 2012-06-18T10:11:20Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1645757) (merged 925a8234 into 0b8b76b). --------------------------------------------------------------------------- by stloyd at 2012-06-18T10:14:52Z @fabpot Anything blocking merge of this PR ? (tests are failing because of issue in master, not releted to this PR) --------------------------------------------------------------------------- by romainneutron at 2012-06-18T10:29:20Z @fabpot @stloyd the latest push was just a rebase push for PR 4577 (symfony/symfony#4577) I'm currently fixing the Exception and changelog things, I'll push very soon --------------------------------------------------------------------------- by romainneutron at 2012-06-18T10:44:38Z @fabpot I've added the exception and the exception interface, add the changelog info --------------------------------------------------------------------------- by travisbot at 2012-06-18T10:53:34Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1645981) (merged 634d6fb9 into 0b8b76b). --------------------------------------------------------------------------- by romainneutron at 2012-06-18T11:08:43Z As reported by @stloyd the PR is failing due to an issue in the master, I re-push and trig the PR build when this issue is solved --------------------------------------------------------------------------- by travisbot at 2012-06-18T11:16:58Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1646006) (merged 2f65945a into 0b8b76b).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The function was failing in a number of different use cases because the function was using a naive character offset comparison to determine where the common path stops.
I've fixed this by splitting up the paths into their individual directories and then comparing the directory names.
If the paths match then the function will return
./and not an empty string. This is for consistency sake, to ensure all returned paths end with/.