Remove oppressive words from code #1205

davidromani · 2020-08-08T19:15:30Z

Subject

We should avoid offending terms like blacklist, whitlist, master, slave... and use more tech-related words like blocklist, primary or secondary, for example.

I am targeting 4.x branch because everything is backwards compatible.

Closes #1191

Changelog

### Changed
- Replace `ip_white_list` by `trusted_ip_list` configuration key

To do

Update the tests;
Update the documentation;
Add an upgrade note.

SonataCI · 2020-08-08T19:24:57Z

Could you please rebase your PR and fix merge conflicts?

docs/reference/two_step_validation.rst

davidromani · 2020-08-09T15:13:10Z

Ok, let me a little bit more time, I prefer to make it BC too...

davidromani · 2020-08-10T09:18:02Z

Hey @greg0ire I don't know how to change this PR against 4.x branch instead of master branch. Is this possible? Or I must open a new PR?

franmomu · 2020-08-10T09:44:04Z

Hey @greg0ire I don't know how to change this PR against 4.x branch instead of master branch. Is this possible? Or I must open a new PR?

You can click on the Edit button on the top right (like changing the title) and then change the base branch.

franmomu · 2020-08-10T11:03:37Z

src/DependencyInjection/SonataUserExtension.php

@@ -178,7 +178,7 @@ public function configureGoogleAuthenticator($config, ContainerBuilder $containe
        }

        $container->setParameter('sonata.user.google.authenticator.forced_for_role', $config['google_authenticator']['forced_for_role']);
-        $container->setParameter('sonata.user.google.authenticator.ip_white_list', $config['google_authenticator']['ip_white_list']);
+        $container->setParameter('sonata.user.google.authenticator.trusted_ip_list', $config['google_authenticator']['trusted_ip_list']);


This would be BC-break, people can still use the ip_white_list option, so here we should also check if ip_white_list exists and use it. A test verifying that this behavior still works would be nice.

PS: Is it possible to deprecate a parameter? 🤔 EDIT: deprecate a parameter in the container.

@franmomu I've marked ip_white_list as a deprecated parameter, is this enough?

ip_white_list is a configuration node. A container parameter is something else entirely. AFAIK You cannot deprecate it, but maybe I'm wrong. IMO @franmomu is right, you should define that parameter from whatever configuration node is defined in the config (after validating that one and only one of them is defined as I described above).

What I meant (about the parameter deprecation in the container) is removing sonata.user.google.authenticator.ip_white_list from the container is BC-break because someone can be using that parameter (it's unlikely, but you never know), if there is no way to deprecate a parameter in the container, an upgrade note I think it's enough and we will remove it on the next major version.

src/DependencyInjection/Configuration.php

greg0ire · 2020-08-10T11:54:09Z

src/DependencyInjection/Configuration.php

@@ -69,6 +69,9 @@ public function getConfigTreeBuilder()
                        ->scalarNode('server')->cannotBeEmpty()->end()
                        ->scalarNode('enabled')->defaultFalse()->end()
                        ->arrayNode('ip_white_list')
+                            ->setDeprecated('The "%node%" option is deprecated. Use "trusted_ip_list" instead with the same values.')
+                        ->end()
+                        ->arrayNode('trusted_ip_list')


It would be great to throw an exception if both configuration nodes are defined. I'm not sure if it can be done here, if yes, that's great, if not you can do it in the extension.

franmomu · 2020-08-10T12:20:22Z

So just to be clear because I didn't explain myself clearly, there are three issues right now:

A user can define ip_white_list and trusted_ip_list, in that case as @greg0ire said it would be great to throw an exception.
In case someone uses ip_white_list, they will get a deprecation message, but it won't work right now, we should do something like:

$trustedIpList = $config['google_authenticator']['ip_white_list'] ?? $config['google_authenticator']['trusted_ip_list'];
$container->setParameter('sonata.user.google.authenticator.trusted_ip_list', trustedIpList);

We shouldn't remove sonata.user.google.authenticator.ip_white_list parameter from the container because is BC-break, so we should add:

$container->setParameter('sonata.user.google.authenticator.ip_white_list', trustedIpList);

davidromani · 2020-08-10T19:04:19Z

@franmomu @greg0ire only as a proof of concept, is the last commit in a good path to solve it?

VincentLanglet · 2020-08-10T19:06:57Z

src/DependencyInjection/SonataUserExtension.php

+        if (array_key_exists('ip_white_list', $config['google_authenticator']) && array_key_exists('trusted_ip_list', $config['google_authenticator'])) {
+            throw new \RuntimeException('Please use only ``trusted_ip_list`` parameter. ``ip_white_list`` is deprecated.');
+        }
+        if (array_key_exists('ip_white_list', $config['google_authenticator']) && !array_key_exists('trusted_ip_list', $config['google_authenticator'])) {
+            $container->setParameter('sonata.user.google.authenticator.trusted_ip_list', $config['google_authenticator']['ip_white_list']);
+        } else {


You should add a comment
// NEXT_MAJOR: Remove this checks and only set the trusted_ip_list.

That's how we deal todos.

src/DependencyInjection/Configuration.php

greg0ire · 2020-08-10T19:08:03Z

src/DependencyInjection/SonataUserExtension.php

-        $container->setParameter('sonata.user.google.authenticator.ip_white_list', $config['google_authenticator']['ip_white_list']);
+
+        if (array_key_exists('ip_white_list', $config['google_authenticator']) && array_key_exists('trusted_ip_list', $config['google_authenticator'])) {
+            throw new \RuntimeException('Please use only ``trusted_ip_list`` parameter. ``ip_white_list`` is deprecated.');


This should be fixed by changing code, so let's use \LogicException here

Suggested change

throw new \RuntimeException('Please use only ``trusted_ip_list`` parameter. ``ip_white_list`` is deprecated.');

throw new \LogicException('Please use only ``trusted_ip_list`` parameter. ``ip_white_list`` is deprecated.');

Also, why the double backticks?

Seen in line 177 of the same class, I've tried to keep same notation (don't know if must be single or double backtick)

This is neither markdown nor rst, don't know why it was contributed like this. IMO we should use double quotes…

greg0ire · 2020-08-10T19:09:09Z

@franmomu @greg0ire only as a proof of concept, is the last commit in a good path to solve it?

It's looking good yeah :)

src/DependencyInjection/Configuration.php

davidromani

I think that now it's all fine tuned, but still getting an error during testing process. Can you help me to solve tests? I don't know very well how to do it.

Thx.

src/DependencyInjection/SonataUserExtension.php

franmomu · 2020-08-12T13:52:09Z

tests/DependencyInjection/ConfigurationTest.php

@@ -42,7 +42,7 @@ public function testDefault(): void
            ],
            'google_authenticator' => [
                'enabled' => false,
-                'ip_white_list' => ['127.0.0.1'],
+                'trusted_ip_list' => ['127.0.0.1'],


Suggested change

'trusted_ip_list' => ['127.0.0.1'],

'ip_white_list' => [],

'trusted_ip_list' => ['127.0.0.1'],

Since ip_white_list is still accepted, it will be in the default values, with this the test should be fixed.

src/DependencyInjection/SonataUserExtension.php

jordisala1991

Can you please squash to a single commit ? If you dont know how to do it, we need to make sure we do a squash merge

davidromani · 2020-08-20T17:30:27Z

Can you please squash to a single commit ? If you dont know how to do it, we need to make sure we do a squash merge

Never done before, but I think that I can do it... let me try it

greg0ire · 2020-08-20T17:31:42Z

Here is a guide:

git rebase -i origin/4.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
A window will show up with many lines, replace pick with fixup on every line but the first one
Close your editor, git should do its magic, and you should end up with one commit
Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

greg0ire · 2020-08-20T18:00:47Z

Looks like you did it! One last thing: please format your commit message according to our rules :

The commit message subject must be less than 50 characters and tell us what you did.
The commit message body should tell us why you did it. It is optional but highly recommended.

Bad example :

Fixed bug #989 by removing call to defraculate()

Good example:

Remove call to defraculate()

Calling this function caused a bug because it interferes 
with calls to getSchmeckles().
Fixes #989

git commit --amend && git push --force-with-lease to do so.

More details in CONTRIBUTING.md

"ip_white_list" from "google_authenticator" configuration key has been replaced by "trusted_ip_list" key. For now, both configuration keys are valid, but only one can be used at same time. "ip_white_list" has been marked as deprecated and will be removed in next major release to keep BC in 4.x branch. Fixes sonata-project#1191

davidromani · 2020-08-21T08:33:25Z

Looks like you did it! One last thing: please format your commit message according to our rules :

All done, thanks for your patience and for teaching me how to improve my Git skills.

codecov-commenter · 2020-08-30T07:22:08Z

Codecov Report

❗ No coverage uploaded for pull request base (4.x@782cdcd). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             4.x    #1205   +/-   ##
======================================
  Coverage       ?   69.61%           
  Complexity     ?      458           
======================================
  Files          ?       46           
  Lines          ?     1415           
  Branches       ?        0           
======================================
  Hits           ?      985           
  Misses         ?      430           
  Partials       ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 782cdcd...4e9613e. Read the comment docs.

franmomu · 2020-08-30T07:26:22Z

Thanks @davidromani!

greg0ire reviewed Aug 8, 2020

View reviewed changes

docs/reference/two_step_validation.rst Show resolved Hide resolved

davidromani changed the base branch from master to 4.x August 10, 2020 10:21

davidromani force-pushed the fix_issue_1191 branch from ab08848 to 9fcb163 Compare August 10, 2020 10:48

davidromani requested a review from greg0ire August 10, 2020 10:55

franmomu requested changes Aug 10, 2020

View reviewed changes

davidromani commented Aug 10, 2020

View reviewed changes

src/DependencyInjection/Configuration.php Show resolved Hide resolved

greg0ire reviewed Aug 10, 2020

View reviewed changes

davidromani requested a review from franmomu August 10, 2020 19:03

VincentLanglet reviewed Aug 10, 2020

View reviewed changes

greg0ire reviewed Aug 10, 2020

View reviewed changes

src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

greg0ire reviewed Aug 10, 2020

View reviewed changes

franmomu reviewed Aug 10, 2020

View reviewed changes

src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

davidromani commented Aug 11, 2020

View reviewed changes

davidromani requested a review from franmomu August 11, 2020 08:12

franmomu reviewed Aug 11, 2020

View reviewed changes

src/DependencyInjection/SonataUserExtension.php Outdated Show resolved Hide resolved

davidromani requested a review from franmomu August 12, 2020 08:03

franmomu reviewed Aug 12, 2020

View reviewed changes

davidromani requested review from franmomu, greg0ire and VincentLanglet August 14, 2020 09:34

greg0ire reviewed Aug 19, 2020

View reviewed changes

src/DependencyInjection/SonataUserExtension.php Outdated Show resolved Hide resolved

davidromani requested a review from greg0ire August 20, 2020 13:31

greg0ire previously approved these changes Aug 20, 2020

View reviewed changes

jordisala1991 previously approved these changes Aug 20, 2020

View reviewed changes

davidromani dismissed stale reviews from jordisala1991 and greg0ire via 90f76c7 August 20, 2020 17:37

davidromani force-pushed the fix_issue_1191 branch 3 times, most recently from ccf5d62 to 634faa2 Compare August 20, 2020 17:43

davidromani force-pushed the fix_issue_1191 branch from 634faa2 to 4e9613e Compare August 21, 2020 07:20

davidromani requested a review from greg0ire August 21, 2020 08:34

jordisala1991 approved these changes Aug 21, 2020

View reviewed changes

greg0ire approved these changes Aug 21, 2020

View reviewed changes

franmomu approved these changes Aug 30, 2020

View reviewed changes

franmomu closed this Aug 30, 2020

franmomu reopened this Aug 30, 2020

franmomu merged commit b8eca6d into sonata-project:4.x Aug 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove oppressive words from code #1205

Remove oppressive words from code #1205

davidromani commented Aug 8, 2020 •

edited

Loading

SonataCI commented Aug 8, 2020

davidromani commented Aug 9, 2020

davidromani commented Aug 10, 2020

franmomu commented Aug 10, 2020

franmomu Aug 10, 2020 •

edited

Loading

davidromani Aug 10, 2020

greg0ire Aug 10, 2020 •

edited

Loading

franmomu Aug 10, 2020 •

edited

Loading

greg0ire Aug 10, 2020

franmomu commented Aug 10, 2020 •

edited

Loading

davidromani commented Aug 10, 2020

VincentLanglet Aug 10, 2020

davidromani Aug 11, 2020

greg0ire Aug 10, 2020

greg0ire Aug 10, 2020

davidromani Aug 11, 2020

greg0ire Aug 19, 2020

greg0ire commented Aug 10, 2020

davidromani left a comment

franmomu Aug 12, 2020

davidromani Aug 14, 2020

jordisala1991 left a comment

davidromani commented Aug 20, 2020

greg0ire commented Aug 20, 2020

greg0ire commented Aug 20, 2020 •

edited

Loading

davidromani commented Aug 21, 2020

codecov-commenter commented Aug 30, 2020 •

edited

Loading

franmomu commented Aug 30, 2020

	throw new \RuntimeException('Please use only ``trusted_ip_list`` parameter. ``ip_white_list`` is deprecated.');
	throw new \LogicException('Please use only ``trusted_ip_list`` parameter. ``ip_white_list`` is deprecated.');

	'trusted_ip_list' => ['127.0.0.1'],
	'ip_white_list' => [],
	'trusted_ip_list' => ['127.0.0.1'],

Remove oppressive words from code #1205

Remove oppressive words from code #1205

Conversation

davidromani commented Aug 8, 2020 • edited Loading

Subject

Changelog

To do

SonataCI commented Aug 8, 2020

davidromani commented Aug 9, 2020

davidromani commented Aug 10, 2020

franmomu commented Aug 10, 2020

franmomu Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

franmomu Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franmomu commented Aug 10, 2020 • edited Loading

davidromani commented Aug 10, 2020

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

greg0ire commented Aug 10, 2020

davidromani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordisala1991 left a comment

Choose a reason for hiding this comment

davidromani commented Aug 20, 2020

greg0ire commented Aug 20, 2020

greg0ire commented Aug 20, 2020 • edited Loading

davidromani commented Aug 21, 2020

codecov-commenter commented Aug 30, 2020 • edited Loading

Codecov Report

franmomu commented Aug 30, 2020

davidromani commented Aug 8, 2020 •

edited

Loading

franmomu Aug 10, 2020 •

edited

Loading

greg0ire Aug 10, 2020 •

edited

Loading

franmomu Aug 10, 2020 •

edited

Loading

franmomu commented Aug 10, 2020 •

edited

Loading

greg0ire commented Aug 20, 2020 •

edited

Loading

codecov-commenter commented Aug 30, 2020 •

edited

Loading