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

[Feature] Add options expansion to Add Lines Configuration #990

Merged

Conversation

mnocon
Copy link
Contributor

@mnocon mnocon commented Aug 10, 2023

Hi!

I've got a chance to play with the new AddLines configurator and it's an extremely useful feature, thanks for introducing it!

There is one small thing that seems missing to me: support for placeholders that are available in the copy-from-package and copy-from-recipe configurators.

It's this feature:

These are the available placeholders: %BIN_DIR%, %CONF_DIR%, %CONFIG_DIR%, %SRC_DIR% %VAR_DIR% and %PUBLIC_DIR%.

Recipes must use these placeholders instead of hardcoding the paths to be truly reusable. The placeholder values can be overridden in the extra section of your composer.json file (where you can define your own placeholders too):

(this text comes from https://github.com/symfony/recipes)

This small PR makes it possible to use placeholders in the add-lines configurator.

I'm not sure about the target branch, please let me know if I should rebase to 1.x - also not sure if I should mention it in the doc somewhere.

@mnocon mnocon force-pushed the expand-options-in-add-lines-configurator branch 2 times, most recently from 1146790 to bc440ea Compare August 10, 2023 14:36
@mnocon mnocon marked this pull request as ready for review August 10, 2023 14:37
@nicolas-grekas
Copy link
Member

Thanks for the PR. Can you please rebase it for branch 1.x? Please also revert the CS changes while doing so as 1.x still supports PHP 7.1

@mnocon mnocon changed the base branch from 2.x to 1.x February 5, 2024 14:15
@mnocon mnocon marked this pull request as draft February 5, 2024 14:15
@mnocon mnocon force-pushed the expand-options-in-add-lines-configurator branch from 042764b to 915e44f Compare February 5, 2024 14:15
@mnocon
Copy link
Contributor Author

mnocon commented Feb 5, 2024

Thanks for the suggestion!
I've rebased this PR onto 1.x and adapted the code to PHP 7.1 (small change: 88672dc)

Questions:

  1. Bot still suggests some CS improvements - but from what I see polyfills are not installed when using --prefer-lowest, is it ok to ignore this?
  2. The --prefer-lowest job is failing with:
[RuntimeException]                                                                                                                                                           
  Could not load package microweber/microweber in [http://packagist.org:](http://packagist.org/) [UnexpectedValueException] Could not parse version constraint v2.6.*: Invalid version string "v2.6.*"

The earliest occurance I found of this failure is here: https://github.com/symfony/flex/actions/runs/7101907556/job/19331086614

I've tried debugging this, but TBH I don't even know where to start (and when running the tests locally I had a different error) - do you have any suggestions where to look?

@mnocon mnocon marked this pull request as ready for review February 5, 2024 15:24
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Nervemind the failures, they're false-positives.

tests/Configurator/AddLinesConfiguratorTest.php Outdated Show resolved Hide resolved
@fabpot fabpot force-pushed the expand-options-in-add-lines-configurator branch from 023c975 to 644e426 Compare February 5, 2024 18:04
@fabpot
Copy link
Member

fabpot commented Feb 5, 2024

Thank you @mnocon.

@fabpot fabpot merged commit 6b46a00 into symfony:1.x Feb 5, 2024
6 of 8 checks passed
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.

5 participants