Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Move SECRETS in a new component 📦️#45571
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Revert "📦 Move secret into a new component"This reverts commit364d22f.📦 Move secret into a new component
carsonbot commentedFeb 27, 2022
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| "symfony/polyfill-mbstring":"~1.0", | ||
| "symfony/filesystem":"^5.4|^6.0", | ||
| "symfony/finder":"^5.4|^6.0", | ||
| "symfony/secret":"^5.4|^6.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be:
| "symfony/secret":"^5.4|^6.0", | |
| "symfony/secret":"^6.1", |
Cause this component will not exists before that release.
| @@ -9,7 +9,9 @@ | |||
| * file that was distributed with this source code. | |||
| */ | |||
| namespaceSymfony\Bundle\FrameworkBundle\Secrets; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Instead of moving you should copy & extend those new files by old ones to have BC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is not a problem that the component is dependent on the FrameworkBundle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is, but we can do differently.
The deprecated framework-bundle classes should extend the new component’s classes, or be aliased to them (see e.g.
| class_exists(BridgeAmqpTransport::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
All classes are flagged@internal. As long assymfony/secret is a required dependency we don't need to use this trick.
But if we decide to makesynfony/secret optional (see my comment#45571 (comment)), in that case, we should keep a BC layer in the FrameworkBundle
| /** | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Most likely should be still there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Even if used in the FramworkBundle ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I also interpret it that no external project may rely on the interfaces of the class. In that case yes, usage in other libraries of the symfony package should work together with the internal tag.
| */ | ||
| namespaceSymfony\Bundle\FrameworkBundle\Secrets; | ||
| declare(strict_types=1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
revert (same comment everywhere)
| 6.1 | ||
| --- | ||
| * added the component |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| *added the component | |
| *Add the component |
| "symfony/polyfill-mbstring":"~1.0", | ||
| "symfony/filesystem":"^5.4|^6.0", | ||
| "symfony/finder":"^5.4|^6.0", | ||
| "symfony/secret":"^6.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO this should be an optional dependency (The FrameworkBundle does notrequire secrets to work).
But I'm not sure how we could provide a BC layer (migration path) without requiring it nor duplicating the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can it be also enabled/disabled via a new framework config key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can it be also enabled/disabled via a new framework config key?
That's already the case.
The point of NOT making it a required dependency is to avoid downloading everything "just in case". Otherwise,symfony/framework-bundle will become the nextsymfony/symfony
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oki I got it thx for explanations :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's make it an optional dependency in 7.0 then? Otherwise we would need to duplicate the code for BC and lose the advantage of the optional dependency in terms of download size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@derrabus agreed, but how triggering a deprecation if the user did not require the deps. Otherwise, bumping symfony to 7.0 will uninstallsymfony/secrets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think, we've had similar problems when we removed messenger transports into bridge packages. How did we solve this issue back then? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We made messenger require the bridges (hard dep) until 6.0
| { | ||
| "name":"symfony/secret", | ||
| "type":"library", | ||
| "description":"Provide a vault to keep sensitive information secret", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what bout adding some keywords as well?
94noni commentedJul 14, 2022
@casahugo hey any news on this PR ? Thank you |
nicolas-grekas commentedJul 29, 2022
I'm sorry to come that late on the proposal but I don't feel like we should add it to Symfony.
I like that and I think that's the way to go. OSS dynamics FTW. 👎 on my side. |
AndreasA commentedAug 2, 2022
@nicolas-grekas would it at least be possible to remove the |
nicolas-grekas commentedAug 3, 2022
Would work for me, please send a PR to have a discussion about this. |
AndreasA commentedAug 3, 2022
@nicolas-grekas Ok. Target branch |
nicolas-grekas commentedAug 3, 2022
6.2 yes |
AndreasA commentedAug 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas Great. Might take me a few weeks to get to it but I think a few weeks to find the time but I guess it doesn't matter as long as it happens before the next LTS release 😄 |
…ctly outside Symfony (AndreasA)This PR was squashed before being merged into the 6.2 branch.Discussion----------[FrameworkBundle] Allow secrets vaults to be used directly outside Symfony| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets || License | MIT| Doc PR |This PR removes the internal tag from the secrets vaults.This allows developers to use them directly without the need to use Symfony config etc. They still have to install the whole framework bundle but that just means an overhead during download, they are still able to directly instantiate the vault using `new SodiumVault(....)` for instance.Furthermore, by removing internal from the AbstractVault, it is possible to add a custom vault and specify it replacing the `secrets.vault` service for instance.See ticket:#44151and previous PR:#45571Commits-------df953f0 [FrameworkBundle] Allow secrets vaults to be used directly outside Symfony
…ctly outside Symfony (AndreasA)This PR was squashed before being merged into the 6.2 branch.Discussion----------[FrameworkBundle] Allow secrets vaults to be used directly outside Symfony| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets || License | MIT| Doc PR |This PR removes the internal tag from the secrets vaults.This allows developers to use them directly without the need to use Symfony config etc. They still have to install the whole framework bundle but that just means an overhead during download, they are still able to directly instantiate the vault using `new SodiumVault(....)` for instance.Furthermore, by removing internal from the AbstractVault, it is possible to add a custom vault and specify it replacing the `secrets.vault` service for instance.See ticket:symfony/symfony#44151and previous PR:symfony/symfony#45571Commits-------df953f038e [FrameworkBundle] Allow secrets vaults to be used directly outside Symfony
…ctly outside Symfony (AndreasA)This PR was squashed before being merged into the 6.2 branch.Discussion----------[FrameworkBundle] Allow secrets vaults to be used directly outside Symfony| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets || License | MIT| Doc PR |This PR removes the internal tag from the secrets vaults.This allows developers to use them directly without the need to use Symfony config etc. They still have to install the whole framework bundle but that just means an overhead during download, they are still able to directly instantiate the vault using `new SodiumVault(....)` for instance.Furthermore, by removing internal from the AbstractVault, it is possible to add a custom vault and specify it replacing the `secrets.vault` service for instance.See ticket:symfony/symfony#44151and previous PR:symfony/symfony#45571Commits-------df953f038e [FrameworkBundle] Allow secrets vaults to be used directly outside Symfony
First commit to move secret in a new component