gitlist-php / gitlist

An elegant and modern git repository viewer
BSD 3-Clause "New" or "Revised" License
5 stars 0 forks source link

Migrate to Silex 2. #4

Closed alehaa closed 7 years ago

alehaa commented 7 years ago

GitList still uses the old API of Silex. In order to be compatible with recent Silex plugins, I suggest porting GitList to Silex 2 API.

Afaik there are no big changes needed, but some of the Interfaces of Silex / Pimple changed.

DannyvdSluijs commented 7 years ago

For who will look into this, for upgrading from Silex 1.x to 2.x there is an article on the Silex wiki here on GitHub. See https://github.com/silexphp/Silex/wiki/Upgrading-Silex-1.x-to-2.x for the article.

PABourdin commented 7 years ago

I wanted to have a look yesterday and found that I would need "ant" on my webserver. This pulls too many (in particular Java) dependencies that I'm not willing to host on a production system. What is the preferred procedure for GitList development?

Edit: Is it enough to successfully build (including tests) on my PC before committing?

PABourdin commented 7 years ago

Silex 2 requires also to update to Symfony 2.8 and Pimple 3.0. I'm currently trying to do that on my PC. I have changed "composer.json", but "composer.lock" seems to be outdated, too.

What do I actually have to do to upgrade those packages? Will I have to download manually and put into "vendors/"?

Thanks for some hints.

PABourdin commented 7 years ago

I could update "composer.json" with:

- "silex/silex": "1.*", + "silex/silex": "2.*",

and have run "php composer.phar update", which generated an updated "composer.lock".

Besides the above mentioned link for "Upgrading-Silex-1.x-to-2.x" one also needs to change the ServiceProvider to use the Pimple-variant like mentioned here: https://stackoverflow.com/questions/43546502/interface-silex-serviceproviderinterface-not-found

alehaa commented 7 years ago

Thanks everybody! I'll work on a PR for this tomorrow. ;)

PABourdin commented 7 years ago

Also "UrlGeneratorServiceProvider" was replaced by "RoutingServiceProvider" as mentioned here: https://stackoverflow.com/questions/38488137/migrate-silex-v1-3-to-v2-classnotfoundexception

PABourdin commented 7 years ago

And "Silex\ControllerProviderInterface" moved to "Silex\Api\ControllerProviderInterface": https://stackoverflow.com/questions/39059782/interface-silex-controllerproviderinterface-not-found-in-controller

PABourdin commented 7 years ago

After implementing the required changes, I get the attached error message from "InterfaceTest.php". I do not know how to continue... If someone wants to have a look, I can send my diffs.

phpunit:
     [exec] PHPUnit 4.1.6 by Sebastian Bergmann.
     [exec] PHP Notice:  Undefined index: projects in /home/philippe/Programme/dev/gitlist/src/Git/Client.php on line 18
     [exec] 
     [exec] Configuration read from /home/philippe/Programme/dev/gitlist/phpunit.xml.dist
     [exec] 
     [exec] The Xdebug extension is not loaded. No code coverage will be generated.
     [exec] 
     [exec] .....F
     [exec] 
     [exec] Time: 484 ms, Memory: 15.75MB
     [exec] 
     [exec] There was 1 failure:
     [exec] 
     [exec] 1) InterfaceTest::testHistoryPage
     [exec] Failed asserting that false is true.
     [exec] 
     [exec] /home/philippe/Programme/dev/gitlist/tests/InterfaceTest.php:272
     [exec]                                       
     [exec] FAILURES!                             
     [exec] Tests: 6, Assertions: 56, Failures: 1.
DannyvdSluijs commented 7 years ago

@PABourdin This would hint to the test failing on line 272. There is a assert on line 272: $this->assertTrue($client->getResponse()->isOk()); it seems something is wrong. Perhaps a var_dump($client->getResponse()->getContent()); die(); would dump the response and give you some hints on what the issue at hand is.

PABourdin commented 7 years ago

@DannyvdSluijs Thanks for the hint. I now receive a GitList error page (HTML output) with this content: 'Oops! Identifier "request" is not defined.' Any idea what this means? Thanks!

DannyvdSluijs commented 7 years ago

@PABourdin sorry guess this could be every thing. I've checked this master brach and it is without error. I would check the changes you've made.

alehaa commented 7 years ago

As mentioned in #10, @p3x-robot already migrated to Silex 2 in his fork. I'll review the changes he made and consider merging his ones.

PABourdin commented 7 years ago

I have finished my work for now, please check @alehaa when you have time.

This is the currently relevant error message:

Pimple\Exception\FrozenServiceException: Cannot override frozen service "twig.app_variable". /home/travis/build/gitlist-php/gitlist/src/Application.php:96

PABourdin commented 7 years ago

The above error is fixed. Tests do pass now. @alehaa after you reviewed my code, you may merge.