Skip to content

Conversation

Zuruuh
Copy link

@Zuruuh Zuruuh commented May 30, 2025

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) May 30, 2025 19:08
Copy link

github-actions bot commented May 30, 2025

Thanks for the PR 😍

How to test these changes in your application

  1. Define the SYMFONY_ENDPOINT environment variable:

    # On Unix-like (BSD, Linux and macOS)
    export SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes-contrib/flex/pull-1811/index.json
    # On Windows
    SET SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes-contrib/flex/pull-1811/index.json
  2. Install the package(s) related to this recipe:

    composer req symfony/flex
    composer req 'carthage-software/mago:^0.24'
  3. Don't forget to unset the SYMFONY_ENDPOINT environment variable when done:

    # On Unix-like (BSD, Linux and macOS)
    unset SYMFONY_ENDPOINT
    # On Windows
    SET SYMFONY_ENDPOINT=

Diff between recipe versions

In order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes.
I'm going keep this comment up to date with any updates of the attached patch.

auto-merge was automatically disabled May 30, 2025 19:23

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) May 30, 2025 19:23
auto-merge was automatically disabled May 30, 2025 19:43

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) May 30, 2025 19:43
auto-merge was automatically disabled May 30, 2025 19:54

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) May 30, 2025 19:54
@Zuruuh
Copy link
Author

Zuruuh commented May 30, 2025

The pipeline is failling because it doesn't support installing composer plugins 🤔 In theory mago could in fact setup it's own config by itself upon installation but I still think using flex here could help to maintain a config with sensible defaults and continue providing upgrades as the configuration's schema changes.

auto-merge was automatically disabled May 30, 2025 20:07

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) May 30, 2025 20:08
Copy link
Member

@diimpp diimpp left a comment

Choose a reason for hiding this comment

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

@azjezz Hi, could you please review if this recipe aligns with https://github.com/carthage-software/mago generic configuration for symfony project.

Personally, I would like for format section to be removed, symfony plugin enabled, bin path removed and php_version somehow resolved at runtime and specified dynamically.

@Zuruuh
Copy link
Author

Zuruuh commented Jun 2, 2025

The format section is exactly the default generated by the command mago init. For the linter section I commented it because it would raise warnings on a default symfony skeleton app 👍 (But yeah we could enable it)


# [linter]
# default_plugins = true
# plugins = ["symfony"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a symfony recipe, i think it makes sense to enable symfony plugin here, maybe php-unit one as well? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I m realizing now that the linter is still enabled, i thought it would be disabled until specifically configured.
Will fix and enable symfony plugin by default. I guess the phpunit one could also be enabled without any runtime performance penalty right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The phpunit plugin is actually super small, there is no performance losse.

What is the reason the default skeleton fails with the symfony plugin? maybe its something we can change in mago?

Copy link
Author

Choose a reason for hiding this comment

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

Default config with symfony and php unit plugins, skeleton app with phpunit installed

config/bundles.php:1:1: warning[strictness/require-strict-types]: Missing `declare(strict_types=1);` statement at the beginning of the file
config/preload.php:1:1: warning[strictness/require-strict-types]: Missing `declare(strict_types=1);` statement at the beginning of the file
public/index.php:7:8: warning[strictness/require-return-type]: Closure is missing a return type hint
public/index.php:1:1: warning[strictness/require-strict-types]: Missing `declare(strict_types=1);` statement at the beginning of the file
tests/bootstrap.php:7:34: warning[best-practices/literal-named-argument]: Literal argument `'bootEnv'` should be passed as a named argument for clarity.
tests/bootstrap.php:12:11: warning[best-practices/literal-named-argument]: Literal argument `0000` should be passed as a named argument for clarity.
tests/bootstrap.php:12:11: warning[migration/explicit-octal-notation]: Use explicit octal numeral notation.
tests/bootstrap.php:1:1: warning[strictness/require-strict-types]: Missing `declare(strict_types=1);` statement at the beginning of the file
src/Kernel.php:1:1: warning[strictness/require-strict-types]: Missing `declare(strict_types=1);` statement at the beginning of the file

This is the tests/bootstrap.php

<?php

use Symfony\Component\Dotenv\Dotenv;

require dirname(__DIR__).'/vendor/autoload.php';

if (method_exists(Dotenv::class, 'bootEnv')) {
    (new Dotenv())->bootEnv(dirname(__DIR__).'/.env');
}

if ($_SERVER['APP_DEBUG']) {
    umask(0000);
}

Copy link
Author

Choose a reason for hiding this comment

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

Now that I think about it I think it could make sense to not report best-practices/literal-named-argument when the function call has only a single argument 🤔 Or maybe it could be configured on the rule level, but maybe it would be a better default ? wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to disable those rules in the recipe? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

idk honestly, I'd say it's best to keep them and to let the choice to the user of either fixing the files or tweaking their linter config to match their preferences

Comment on lines +15 to +18
[format]
print_width = 120
tab_width = 4
use_tabs = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Those are indeed the defaults, mago init generates this just to make it obvious where formatting settings go, it could be removed, or commented out with a link to the formatter docs? ref: https://docs.rs/mago-formatter/latest/mago_formatter/settings/struct.FormatSettings.html

auto-merge was automatically disabled June 4, 2025 23:11

Head branch was pushed to by a user without write access

@symfony-recipes-bot symfony-recipes-bot enabled auto-merge (squash) June 4, 2025 23:11
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.

3 participants