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.

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

Symfony 5 support #1045

Closed
wants to merge 30 commits into from
Closed

Symfony 5 support #1045

wants to merge 30 commits into from

Conversation

jmontoyaa
Copy link
Contributor

  • Fix php unit tests
  • Add PHP 7.2 and 7.3 in travis - (remove PHP 5.6 in travis)

@jmontoyaa jmontoyaa changed the title Upgrade twig version Update twig version Dec 17, 2019
@jmontoyaa
Copy link
Contributor Author

I'm preparing another change to add support to Symfony 5

@Thunderbird69
Copy link

SF5 support would be much appreciated ;) Thanks a bunch in advance

@jmontoyaa jmontoyaa changed the title Update twig version Symfony 5 support Feb 18, 2020
@npotier npotier added this to the 4.X milestone Apr 1, 2020
composer.json Outdated
@@ -21,26 +21,26 @@
],
"require": {
"php": ">=5.6",
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the composer.json maintains PHP 5.6 as minimum, shouldn't we keep the travis-ci tests with this version ?

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 is not worthy anymore to support PHP5.6 but, if we do, then yes, travis matrix should reflect this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then which version of PHP will be supported? From PHP 7.2?

Copy link
Member

Choose a reason for hiding this comment

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

As this bundle was "frozen" for a while, I suspect that go with 7.2 is not that crime. If we want, we could keep 7.*, don't know. As i "left" the (active) support for this bundle, I would not take a decision.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we follow Symfony requirements, I think we should have a version (3.X) compatible with Symfony 3 / PHP 5.6 and another (4.X) compatible with Symfony 4 / PHP 7.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an error when installing with PHP 7.1 (see travis ):

Because in composer.json there is:

"doctrine/mongodb-odm": "^2.0"

Error message:

- doctrine/mongodb-odm 2.0.3 requires php ^7.2 -> your PHP version (7.1.27) does not satisfy that requirement.

If I change it to "doctrine/mongodb-odm": "^1.0" then it will work, but this will downgrade some symfony packages to version 4.

Copy link
Member

Choose a reason for hiding this comment

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

So, in order to keep all the compatibilities, we should require php 7.2 as minimum php version (as I suspected). TBH I don't know if still keep a mongo support is a good idea, but as long as this bundle is really a mess of code, I would go with php7.2.
I mean, this bundle does not followed - in past - good practices both in code and in management. I think it would be hard to fix all the things at the same time.
Just my two cents, of course.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so let me get this straight : the idea would be to have a new minor version of the bundle (3.3.0) with PHP 7.2 as a minimum version in composer.json and still a Symfony 3 compatibility ?

Copy link
Member

Choose a reason for hiding this comment

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

Kind of

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 will send those changes, add check what travis says :D

@pierrotevrard-dbit
Copy link

Got an error with this PR: Attempted to call an undefined method named "renderResponse" of class "Twig\Environment".
in vendor/apy/datagrid-bundle/Grid/Grid.php (line 2152)

The method does not exists anymore and must be replaced by a code like:

        $content = $this->container->get('twig')->renderView($view, $parameters);

        if (null === $response) {
            $response = new Response();
        }

        $response->setContent($content);

        return $response;

@pierrotevrard-dbit
Copy link

I've finally made a branch for Twig 3.1 compatibility, see https://github.com/pierrotevrard-dbit/APYDataGridBundle/tree/fix/twig_31_compatibility

It does not seem backward compatible with Twig 2.0, sry

kaamaa and others added 9 commits March 12, 2021 21:09
Changes to run with PHP 7.4 to prevent the following error Message:
Trying to access array offset on value of type null
replace spaceless by apply spaceless in blocks.html.twig
FredDut and others added 7 commits August 12, 2021 16:34
Fix "get('templating')" as in grid.php
spaceless deprecated in blocks.html.twig
spaceless in blocks.html.twig
Change path for the exporter to work on SF5.2
@npotier
Copy link
Collaborator

npotier commented Sep 13, 2021

Hello,

I've been working on this branch https://github.com/APY/APYDataGridBundle/tree/feature/sf5-compat to have a working Bundle with latest version on Symfony 5.

All tests are green and I've created a real life example that also allows to test the bundle locally : https://github.com/npotier/apydatagrid-demo

I've created a PR here : #1063

Feel free to tell me what you think about it.

@FredDut
Copy link
Contributor

FredDut commented Sep 13, 2021

Hello,
It's a nice job.
I think something is missing: when you filter, the column is filtered but the value disappears from the input box.
Regards

FredDut and others added 2 commits October 21, 2021 08:45
invert column exists test in grid_column_filter_type_select and grid_column_filter_type_input
Update blocks.html.twig
@FredDut
Copy link
Contributor

FredDut commented Oct 21, 2021

Hello ,
Do you plan to merge fixes from jmontoya in sf5-compat branch?
I can't see fixes for Export.php (630a1b5) , GridManager.php (56c62e1) and spaceless in blocks.html.twig (9425cd1) in sf5-compat branch. Maybe I misunderstand something.

@FredDut
Copy link
Contributor

FredDut commented Nov 16, 2021

I think the datagrid is OK for php7.2 with this fixes.
With php 7.4 it's not ok. When you extend the grid with custom twigs, columns or data are not initialized. Same problem for exports. I don't know what to do with this.

npotier added a commit that referenced this pull request Jan 19, 2022
npotier added a commit that referenced this pull request Jan 19, 2022
add fixes related to pull #1045 conversation
@npotier
Copy link
Collaborator

npotier commented Jan 19, 2022

Hello @FredDut Thank you for your message. I've integrated the changes in the 4.0.1 tag of the bundle.

Unfortunately, I don't have access to packagist for this bundle in order to trigger an update

@npotier
Copy link
Collaborator

npotier commented Feb 10, 2023

I think we can close this PR, as the modifications has been integrated in new versions. Feel free to add comment if necessary

@npotier npotier closed this Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants