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

Add support for Symfony 7 #28

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mbrodala
Copy link

@mbrodala mbrodala commented Jan 4, 2024

No description provided.

@mbrodala
Copy link
Author

@danepowell Could you have a look?

@mbrodala mbrodala marked this pull request as ready for review January 15, 2024 16:30
@danepowell
Copy link
Collaborator

@mbrodala have you actually tested this with Symfony 7? I know there are many changes, such as the use of PHP attributes for command names, which this package is not up to date with.

@mbrodala
Copy link
Author

mbrodala commented Jan 17, 2024

These are optional features, the current code actually is fine AFAIS. As a simple check I did a Rector dry-run on this project:

1 file with changes
===================

1) src/SelfUpdateCommand.php:57

    ---------- begin diff ----------
@@ @@
         // Follow Composer's pattern of command and channel names.
         $this
             ->setAliases(array('update', 'self-update'))
-            ->setDescription("Updates $app to the latest version.")
             ->addArgument('version_constraint', InputArgument::OPTIONAL, 'Apply version constraint')
             ->addOption('stable', NULL, InputOption::VALUE_NONE, 'Use stable releases (default)')
             ->addOption('preview', NULL, InputOption::VALUE_NONE, 'Preview unstable (e.g., alpha, beta, etc.) releases')
@@ @@
      *
      * @throws \Exception
      */
-    protected function execute(InputInterface $input, OutputInterface $output)
+    protected function execute(InputInterface $input, OutputInterface $output): int
     {
         if (!$this->ignorePharRunningCheck && empty(\Phar::running())) {
             throw new \Exception(self::SELF_UPDATE_COMMAND_NAME . ' only works when running the phar version of ' . $this->applicationName . '.');
@@ @@
         ]);
         if (null === $latestRelease || Comparator::greaterThanOrEqualTo($this->currentVersion, $latestRelease['version'])) {
             $output->writeln('No update available');
-            return 0;
+            return \Symfony\Component\Console\Command\Command::SUCCESS;
         }

         $fs = new sfFilesystem();
@@ @@
             $output->writeln('<error>The download is corrupted (' . $e->getMessage() . ').</error>');
             $output->writeln('<error>Please re-run the self-update command to try again.</error>');

-            return 1;
+            return \Symfony\Component\Console\Command\Command::FAILURE;
         }
+        return \Symfony\Component\Console\Command\Command::SUCCESS;
     }

     /**
    ----------- end diff -----------

Applied rules:
 * ConsoleExecuteReturnIntRector (https://github.com/symfony/symfony/pull/33775/files)
 * CommandConstantReturnCodeRector
 * AddReturnTypeDeclarationRector


                                                                                                                        
 [OK] 1 file would have changed (dry-run) by Rector                                                                     
                                                                                                                        
rector.php
<?php

declare(strict_types=1);

use Rector\Config\RectorConfig;
use Rector\Symfony\Set\SymfonyLevelSetList;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->paths([
        __DIR__.'/src',
    ]);

    $rectorConfig->sets([
        SymfonyLevelSetList::UP_TO_SYMFONY_64,
    ]);
};

So nothing really which must be changed for Symfony 7 compat.

As for your question: no, I did not yet test this with Symfony 7 since this is currently actually a blocker for one of our projects which uses gordalina/cachetool. I'll try to test this.

@danepowell
Copy link
Collaborator

I think we should remove support for everything except Symfony 5.4, 6.4, and 7, given the EOL timelines. And maybe make all of the possible rector upgrades at the same time (as long as they're backwards-compatible to 5.4).

@mbrodala
Copy link
Author

Agreed, that's what I opened #29 for. :-)

@greg-1-anderson
Copy link
Member

This all sounds good to me as well; please be sure to bump the self-update major version when removing support for older Symfony versions. I would recommend removing support for older PHP versions at the same time this is done, maybe retaining support for 7.4+ (although I would have no objection to removing 7 completely).

@danepowell
Copy link
Collaborator

I removed support for EOL PHP and Symfony versions in #30

Once someone has tested the current PR on Symfony 7, let me know and I can merge these together.

@greg-1-anderson
Copy link
Member

Looking at this again, I see that the comment from 16 Jan includes:

protected function execute(InputInterface $input, OutputInterface $output): int

This looks like a change that would be necessary to make in Symfony 7. It is probably okay to include the return typehint with earlier versions of Symfony, as PHP seems to be forgiving about adding return typehints that do not exist in the parent class. Removing return typehints that are in the parent class is not allowed without a "ReturnTypeMightChange" annotation, though (which is only supported in PHP 8.1+).

The tests only run with PHP 7.4, if I am not mistaken, so Symfony 7 won't be covered in the current tests.

@greg-1-anderson greg-1-anderson mentioned this pull request Apr 8, 2024
@mbrodala
Copy link
Author

mbrodala commented Apr 8, 2024

@greg-1-anderson See #30 which resolves this by dropping support for PHP 7.x and 8.0 among others.

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.

None yet

3 participants