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

Upgrade Symfony 6.0 dependencies #1914

Conversation

fabianoroberto
Copy link
Contributor

On this PR:

@fabianoroberto
Copy link
Contributor Author

Check failed because api-platform has not released a version supporting Symfony 6 yet (they are working on this).
There is an issue for that api-platform/core#4543

@dougetovski
Copy link

dougetovski commented Dec 9, 2021

We should also keep in mind that Symfony 6 will define return types. Only updating composer.json to accepts ^6.0 without defining types on Symfony methods that NelmioApiDocBundle override may cause problems.
Correct me if i'm wrong.

@fabianoroberto
Copy link
Contributor Author

@dougetovski you're absolutely right, my fault.
Maybe we could use Rector (https://github.com/rectorphp/rector) to plan the migration.
I'll try ASAP

encreinformatique and others added 5 commits December 11, 2021 13:58
* fixed : add doc blocks to silence Symfony deprecations

Symfony 5.4 returning deprecations about 
```
User Deprecated: Method "Twig\Extension\ExtensionInterface::getFunctions()" might add "array" as a native return type declaration in the future.
Do the same in implementation "Nelmio\ApiDocBundle\Render\Html\GetNelmioAsset" now to avoid errors or add an explicit @return annotation to suppress this message.
```

* fixed : native hint
* add zircore/swagger-php v4 to composer.json

* fix incompatibilities

* add compatibility with 3.2

* Apply fixes from StyleCI

* mark SetsContextTrait as internal

* Bump php version

Co-authored-by: Alexey <alshenestky@icloud.com>
Co-authored-by: Alexey Alshenetsky <alshenetsky@users.noreply.github.com>
Co-authored-by: Guilhem Niot <guilhem@gniot.fr>
* Adding ``header_block`` Twig Block

In attempting to remove the default green header from the API docs (I am integrating the docs into a site that already has a header), I have found that the only possible way to do so is to overwrite the entire ``index.html.twig`` file.  This is because the existing ``{% header %}`` twig tags are placed *inside* the ``<header>`` HTML tags.  So, if one overrides the ``{% header %}`` block, it only adjusts the header content, as opposed to allowing control over the entire header object.

To resolve this problem I considered two methods:

- Move the existing ``{% header %}`` tags _*outside*_ of the ``<header>`` tags.  This would resolve the problem in the simplest fashion, but would be a breaking change for those currently overriding the block.

- Add a new twig block, placing its tags *_outside_* the existing ``<header>`` tags.  This would leave the existing functionality as-is, and would not break any current implementations.

I chose the latter for the reasons specified above, and suggested the name ``{% header_block %}``.  Thanks for considering this proposal.

* Add Location of Original Twig Template

This doc page advises that one can `have a look at the original template to see which blocks can be overridden`, but does not say where the original template can be found.  This edit adds that information so that users don't have to resort to searching for keywords (like I just had to).

Note that I haven't run any tests as this is a docs-only change.
alshenetsky and others added 5 commits December 12, 2021 01:32
…elmio#1918)

* add null check

nelmio#1909

* less code is better

* add tests for ControllerReflection::getReflectionMethod()

* lint fix

* style_ci fixes

Co-authored-by: Alexey <alshenestky@icloud.com>
…dependencies' into feature/1913-upgrate-symfony-60-dependencies

# Conflicts:
#	.github/workflows/continuous-integration.yml
#	composer.json
@@ -15,41 +15,41 @@
}
],
"require": {
"php": ">=7.1.3",
"php": ">=8.0",
Copy link
Collaborator

@GuilhemN GuilhemN Dec 14, 2021

Choose a reason for hiding this comment

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

I don't think we should bump the php version yet, PHP 7 is still widely used (see https://blog.packagist.com/php-versions-stats-2021-1-edition/).
Moreover unlike Symfony, we don't have enough resources to keep providing bug fixes to older versions for people still using php 7.x, so I'd rather stick with a constraint allowing to provide bug fixes to most of the users of this bundle with minimum maintenance load.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. Maybe could be drop support for php <7.4 (https://www.php.net/supported-versions.php) and symfony <5.3 (https://symfony.com/releases)

"ext-json": "*",
"doctrine/annotations": "^1.11",
"psr/cache": "^1.0|^2.0|^3.0",
"psr/container": "^1.0|^2.0",
"psr/log": "^1.0|^2.0|^3.0",
"symfony/config": "^4.4|^5.0",
Copy link
Collaborator

@GuilhemN GuilhemN Dec 14, 2021

Choose a reason for hiding this comment

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

Is it making things harder to keep supporting Symfony 4.4?
If not, I'd rather keep supporting it, the argument is the same as for PHP 7.x, I think we should keep our constraints as loose as possible, both to simplify the maintenance work, and provide bug fixes to more users :)

(And actually Symfony 4.4 will receive bug fixes until the end of 2023)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above, I can reintroduce support for ^4.4|^5.3|^6.0, but I need revert some useful changes like str_* or union types available as of PHP 8.0.0.

@@ -20,46 +20,37 @@
*/
class ControllerReflector
{
private $container;
private mixed $controllerNameParser;
Copy link

Choose a reason for hiding this comment

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

This should have a default value (null) or the line 67 will lead to a fatal error (if ($this->controllerNameParser)


/**
* @var string[]
*/
public $groups;
public array $groups;
Copy link

Choose a reason for hiding this comment

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

This will lead to fatal errors too: only $type is required.

@b1rdex
Copy link

b1rdex commented Dec 21, 2021

There are tons of hidden fatal errors. I'd recommend splitting the MR to a) allowing sf 6.0 and then b) upgrading to PHP 8 if necessary.

Also, introducing some static code analysis (PHPStan or Psalm) would ease PHP 8.0 upgrade, helping to find the errors.

@michaljusiega
Copy link

I also thinking about spliting this PR.

For me now, this bundle is blocker to bump my projects with Symfony 6.0.

@GuilhemN
Copy link
Collaborator

I merged #1933 with your changes limited to the upgrade to symfony 6.0.
We should be all good, all that remains is to make a release of it :)

For migrating to PHP 8 and Symfony > 5.3, I'm still not in favor of it, this is not necessary and wouldn't really ease the maintenance so for me it should be delayed.

@GuilhemN
Copy link
Collaborator

I'm closing this as its main goal is fulfilled, feel free to reopen this PR or a new one if you feel the need.

@GuilhemN GuilhemN closed this Dec 29, 2021
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.

no support for symfony 6
9 participants