Move SECRETS in a new component 📦️ #45571

casahugo · 2022-02-27T14:21:15Z

First commit to move secret in a new component

Q	A
Branch?	6.1 for features
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #44151
License	MIT
Doc PR	symfony/symfony-docs#...

Revert "📦 Move secret into a new component" This reverts commit 364d22f. 📦 Move secret into a new component

carsonbot · 2022-02-27T14:21:22Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 6.1 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

stloyd · 2022-02-27T14:41:52Z

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -29,6 +29,7 @@
        "symfony/polyfill-mbstring": "~1.0",
        "symfony/filesystem": "^5.4|^6.0",
        "symfony/finder": "^5.4|^6.0",
+        "symfony/secret": "^5.4|^6.0",


This should be:

Suggested change

"symfony/secret": "^5.4|^6.0",

"symfony/secret": "^6.1",

Cause this component will not exists before that release.

stloyd · 2022-02-27T15:17:08Z

src/Symfony/Component/Secret/AbstractVault.php

@@ -9,7 +9,9 @@
 * file that was distributed with this source code.
 */

-namespace Symfony\Bundle\FrameworkBundle\Secrets;


Instead of moving you should copy & extend those new files by old ones to have BC.

It is not a problem that the component is dependent on the FrameworkBundle?

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.

symfony/src/Symfony/Component/Messenger/Transport/AmqpExt/AmqpTransport.php

Line 18 in fdfa40e

class_exists(BridgeAmqpTransport::class);

).

All classes are flagged @internal. As long as symfony/secret is a required dependency we don't need to use this trick.

But if we decide to make synfony/secret optional (see my comment #45571 (comment)), in that case, we should keep a BC layer in the FrameworkBundle

stloyd · 2022-02-27T15:17:36Z

src/Symfony/Component/Secret/Command/SecretsDecryptToLocalCommand.php


 /**
 * @author Nicolas Grekas <p@tchwork.com>
- *
- * @internal


Most likely should be still there.

Even if used in the FramworkBundle ?

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.

jderusse · 2022-02-27T17:23:33Z

src/Symfony/Component/Secret/AbstractVault.php

@@ -9,12 +9,12 @@
 * file that was distributed with this source code.
 */

-namespace Symfony\Bundle\FrameworkBundle\Secrets;
+declare(strict_types=1);


revert (same comment everywhere)

jderusse · 2022-02-27T17:36:46Z

src/Symfony/Component/Secret/CHANGELOG.md

+6.1
+---
+
+* added the component


Suggested change

* added the component

* Add the component

jderusse · 2022-02-27T17:42:57Z

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -29,6 +29,7 @@
        "symfony/polyfill-mbstring": "~1.0",
        "symfony/filesystem": "^5.4|^6.0",
        "symfony/finder": "^5.4|^6.0",
+        "symfony/secret": "^6.1",


IMHO this should be an optional dependency (The FrameworkBundle does not require secrets to work).

But I'm not sure how we could provide a BC layer (migration path) without requiring it nor duplicating the code.

can it be also enabled/disabled via a new framework config key?

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 next symfony/symfony

oki I got it thx for explanations :)

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.

@derrabus agreed, but how triggering a deprecation if the user did not require the deps. Otherwise, bumping symfony to 7.0 will uninstall symfony/secrets

Sounds good to me

I think, we've had similar problems when we removed messenger transports into bridge packages. How did we solve this issue back then? 🤔

We made messenger require the bridges (hard dep) until 6.0

94noni · 2022-02-28T10:51:14Z

src/Symfony/Component/Secret/composer.json

+{
+    "name": "symfony/secret",
+    "type": "library",
+    "description": "Provide a vault to keep sensitive information secret",


what bout adding some keywords as well?

94noni · 2022-07-14T17:21:43Z

@casahugo hey any news on this PR ? Thank you

nicolas-grekas · 2022-07-29T13:13:48Z

I'm sorry to come that late on the proposal but I don't feel like we should add it to Symfony.
Creating a new component would increase the maintenance and the responsibility of the project and I don't think this is desired on this topic.

I created a repository to extract the implementation of the symfony secrets. I just copied the code but it's not a viable long term alternative: casahugo/secrets

I like that and I think that's the way to go. OSS dynamics FTW.

👎 on my side.

AndreasA · 2022-08-02T17:51:30Z

@nicolas-grekas would it at least be possible to remove the @internal from the secret vault classes? that way they could be used directly, even though the full framework would need to be downloaded?

nicolas-grekas · 2022-08-03T08:14:20Z

Would work for me, please send a PR to have a discussion about this.

AndreasA · 2022-08-03T18:41:42Z

@nicolas-grekas Ok. Target branch 6.2?

nicolas-grekas · 2022-08-03T19:23:07Z

6.2 yes

AndreasA · 2022-08-09T17:20:15Z

@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: #44151 and previous PR: #45571 Commits ------- 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#44151 and previous PR: symfony/symfony#45571 Commits ------- df953f038e [FrameworkBundle] Allow secrets vaults to be used directly outside Symfony

📦 Move secret into a new component

745cd3d

Revert "📦 Move secret into a new component" This reverts commit 364d22f. 📦 Move secret into a new component

carsonbot added Status: Needs Review Feature labels Feb 27, 2022

carsonbot added this to the 6.1 milestone Feb 27, 2022

stloyd reviewed Feb 27, 2022

View reviewed changes

casahugo added 2 commits February 27, 2022 17:16

🔧 update secret config

a792acb

🔧 set component version to 6.1

7b90d00

jderusse reviewed Feb 27, 2022

View reviewed changes

94noni reviewed Feb 28, 2022

View reviewed changes

chalasr modified the milestones: 6.1, 6.2 May 8, 2022

nicolas-grekas closed this Jul 29, 2022

nicolas-grekas mentioned this pull request Jul 29, 2022

[RFC] Move SECRETS in a new components📦️ #44151

Closed

casahugo deleted the secret-component branch August 15, 2022 11:51

AndreasA mentioned this pull request Sep 1, 2022

[FrameworkBundle] Allow secrets vaults to be used directly outside Symfony #47445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move SECRETS in a new component 📦️ #45571

Move SECRETS in a new component 📦️ #45571

casahugo commented Feb 27, 2022

carsonbot commented Feb 27, 2022

stloyd Feb 27, 2022

stloyd Feb 27, 2022

casahugo Feb 27, 2022 •

edited

Loading

chalasr Feb 27, 2022

jderusse Feb 27, 2022

stloyd Feb 27, 2022

casahugo Feb 27, 2022

simonberger Mar 2, 2022

jderusse Feb 27, 2022

jderusse Feb 27, 2022

jderusse Feb 27, 2022

94noni Feb 28, 2022

jderusse Feb 28, 2022

94noni Feb 28, 2022

derrabus Feb 28, 2022 •

edited

Loading

jderusse Feb 28, 2022

chalasr Feb 28, 2022

derrabus Feb 28, 2022

chalasr May 4, 2022

94noni Feb 28, 2022

94noni commented Jul 14, 2022

nicolas-grekas commented Jul 29, 2022

AndreasA commented Aug 2, 2022

nicolas-grekas commented Aug 3, 2022

AndreasA commented Aug 3, 2022

nicolas-grekas commented Aug 3, 2022

AndreasA commented Aug 9, 2022 •

edited

Loading

Move SECRETS in a new component 📦️ #45571

Move SECRETS in a new component 📦️ #45571

Conversation

casahugo commented Feb 27, 2022

carsonbot commented Feb 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casahugo Feb 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

94noni commented Jul 14, 2022

nicolas-grekas commented Jul 29, 2022

AndreasA commented Aug 2, 2022

nicolas-grekas commented Aug 3, 2022

AndreasA commented Aug 3, 2022

nicolas-grekas commented Aug 3, 2022

AndreasA commented Aug 9, 2022 • edited Loading

casahugo Feb 27, 2022 •

edited

Loading

derrabus Feb 28, 2022 •

edited

Loading

AndreasA commented Aug 9, 2022 •

edited

Loading