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.

Sign up for GitHub

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

Jump to bottom

Upgrade to Symfony4.4 LTS with PHP7.2 support #307

Merged
merged 6 commits into from
Jul 28, 2020
Merged

Upgrade to Symfony4.4 LTS with PHP7.2 support #307

merged 6 commits into from
Jul 28, 2020

Conversation

pablothedude
Copy link
Contributor

@pablothedude pablothedude commented Jun 2, 2020 •
edited
Loading

https://www.pivotaltracker.com/story/show/174036854

@pablothedude pablothedude force-pushed the feature/test-php-7-2-in-travis branch from 30e6de9 to bdb35a7 Compare June 2, 2020 07:58
@pablothedude pablothedude force-pushed the feature/test-php-7-2-in-travis branch 4 times, most recently from a274984 to 8b30a29 Compare June 18, 2020 12:25
@pablothedude pablothedude changed the title Test PHP 7.2 with Travis Upgrade to Symfony4.4 LTS with PHP7.2 support Jun 18, 2020
@pablothedude pablothedude changed the title Upgrade to Symfony4.4 LTS with PHP7.2 support Upgrade to Symfony4.4 lts with PHP7.2 support Jun 18, 2020
@pablothedude pablothedude changed the title Upgrade to Symfony4.4 lts with PHP7.2 support Upgrade to Symfony4.4 LTS with PHP7.2 support Jun 18, 2020
@pablothedude pablothedude force-pushed the feature/test-php-7-2-in-travis branch 3 times, most recently from 7b8a2df to b4c690a Compare June 22, 2020 09:47
@pablothedude pablothedude requested a review from MKodde June 22, 2020 09:50
MKodde
MKodde reviewed Jun 22, 2020
View reviewed changes
Copy link
Member

@MKodde MKodde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite the change! I'm going to give this a functional testrun now. This may result in additional feedback. Most of my findings are cosmetic, but also touch on some more serious points.

One of the things I noticed in the updates of the tests is that almost none of the expected exception classes are 'imported'.

.gitignore Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
bin/doctrine-migrations-diff.sh Outdated
Comment on lines 3 to 5
php72 ./bin/console do:mi:di --em=middleware --filter-expression='~^(?!event_stream).*$~'
sleep 1
php72 ./bin/console do:mi:di --em=gateway
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this change in behaviour (replacing the dedicated GW/MW doctrine console actions) should be documented in a dev readme.

Also consider not using the abbreviated calling of the console actions. Doesn't hurt either, but looks somewhat messy IMHO.

Finally, the file is missing a newline at the end of the file.

composer.json Outdated Show resolved Hide resolved
config/legacy/parameters.yaml Outdated Show resolved Hide resolved
config/routes/annotations.yaml
Comment on lines +1 to +7
#controllers:
# resource: ../../src/Controller/
# type: annotation
#
#kernel:
# resource: ../../src/Kernel.php
# type: annotation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this file be removed?

config/bootstrap.php
@@ -0,0 +1,25 @@
<?php

use Symfony\Component\Dotenv\Dotenv;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import is currently not used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to manage it with Symfony itself.

src/Surfnet/DataFixtures/AppFixtures.php Outdated
Comment on lines 12 to 13
// $product = new Product();
// $manager->persist($product);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps remove these commented lines?

templates/base.html.twig
@@ -1,10 +1,9 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8" />
<meta charset="UTF-8">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/Surfnet/Stepup/Tests/Configuration/Value/InstitutionAuthorizationOptionTest.php
* @dataProvider invalidConstructorArgumentsProvider
*/
public function invalid_types_are_rejected_during_construction($arguments)
{
$this->expectException(\Surfnet\Stepup\Exception\InvalidArgumentException::class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All $this->expectException Exception references [sh/c]ould be 'imported'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know but is was easier to replace it without refactoring all the code. Because it's in the test code I didn't bother to refactor it.

MKodde
MKodde reviewed Jun 22, 2020
View reviewed changes
Copy link
Member

@MKodde MKodde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README.md also needs to be updated. This states the application can be run using PHP 5.6. As of now that is no longer true.

MKodde
MKodde reviewed Jun 22, 2020
View reviewed changes
src/Surfnet/DataFixtures/AppFixtures.php Outdated
namespace Surfnet\DataFixtures;

use Doctrine\Bundle\FixturesBundle\Fixture;
use Doctrine\Common\Persistence\ObjectManager;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the ObjectManager from Doctrine\Common\Persistence\ is incompatible with super

MKodde
MKodde reviewed Jun 22, 2020
View reviewed changes
composer.json
"doctrine/orm": "~2.5",
"sensio/distribution-bundle": "~5.0",
"sensio/framework-extra-bundle": "~3.0",
"ext-json": "*",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ext-openssl and ext-pdo also are required extensions. Please add those to the require section

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's of out of scope for the upgrade when I'm touching the code when working on other features I'll boyscout this.

@pablothedude pablothedude force-pushed the feature/test-php-7-2-in-travis branch 2 times, most recently from 80b9450 to 3cb1bdc Compare July 3, 2020 12:51
@pablothedude pablothedude force-pushed the feature/test-php-7-2-in-travis branch 2 times, most recently from 5078304 to 4994d46 Compare July 3, 2020 15:52
@pablothedude
Remove encrypted cookies
87f6124 
Mcrypt is deprecated
@pablothedude pablothedude force-pushed the feature/test-php-7-2-in-travis branch from 4994d46 to 87f6124 Compare July 3, 2020 16:01
@pablothedude pablothedude mentioned this pull request Jul 17, 2020
Merged
1 task
@pablothedude
Copy link
Contributor Author

Some changes were added after functional testing

@pablothedude pablothedude requested a review from MKodde July 24, 2020 10:11
@pablothedude pablothedude merged commit c1b94b6 into develop Jul 28, 2020
@MKodde MKodde deleted the feature/test-php-7-2-in-travis branch July 4, 2022 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MKodde MKodde Awaiting requested review from MKodde

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pablothedude @MKodde