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

[1.10] Replace whitelist with allow list #8957

Merged
merged 3 commits into from Jun 16, 2020
Merged

[1.10] Replace whitelist with allow list #8957

merged 3 commits into from Jun 16, 2020

Conversation

GrahamCampbell
Copy link
Contributor

@GrahamCampbell GrahamCampbell commented Jun 7, 2020

White/blacklist is outdated and offensive terminology, and we should no longer support its use. This PR aims to be BC compatible for 1.10.x, and can be followed up in 2.0.x with full removal.

@GrahamCampbell GrahamCampbell marked this pull request as ready for review Jun 7, 2020
@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 7, 2020

The failing tests are unrelated to this PR (already failing on the 1.10 branch as is).

doc/01-basic-usage.md Outdated Show resolved Hide resolved
@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 7, 2020

I think the notion of allowing a version is closest to whitelisting. yesno does not capture that.

@browner12
Copy link

browner12 commented Jun 7, 2020

These terms don't appear to have racial origins, but maybe that connotation has been attached to them in present day, and if that's the case I'm for replacing them.

Here is a similar undertaking in the Chromium project.

https://bugs.chromium.org/p/chromium/issues/detail?id=981129

They are suggesting the terms "allowlist" and "blocklist".

->setWhitelistAllDependencies($input->getOption('update-with-all-dependencies'))
->setUpdatAllowList(array_keys($requirements))
->setAllowListTransitiveDependencies($input->getOption('update-with-dependencies'))
->setAllowListAllDependencies($input->getOption('update-with-all-dependencies'))
Copy link
Member

@Seldaek Seldaek Jun 8, 2020

Choose a reason for hiding this comment

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

Please undo the changes to Installer methods entirely. It's already been fixed in master and this will cause conflicts for little gain really..

@Seldaek
Copy link
Member

Seldaek commented Jun 8, 2020

Generally speaking all the method changes I'd rather not have them in this PR. Same for the changes in test files. 1.10 is maintenance only. Rather change this in master branch directly.

Docs/strings changes which are publicly visible can be changed already in this PR as this has more impact towards end users. It's more value/less conflicts. Win-win.

@Seldaek
Copy link
Member

Seldaek commented Jun 8, 2020

As for wording, please note that master branch already uses allowlist in many places, so yeah rather keep using allow/allow list as variant to whitelist. Blacklist is less present generally, and the change to exclude for AutoloadGenerator makes complete sense and is in line with exclude-from-classmap in the autoload config too 👍 .

@Seldaek Seldaek added this to the 1.10 milestone Jun 8, 2020
@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 8, 2020

The reason I changed the internals is because we need to be as inclusive as possible to contributors, and not just to users who don't look at the source code.

As for merge conflicts, I am happy to resolve them, in a 1.10 -> master merge PR.

Co-authored-by: ZhangWei <zhwei.yes@gmail.com>
@northys
Copy link

northys commented Jun 8, 2020

I'm happy to see that people have lost their mind

@theRealCooller
Copy link

theRealCooller commented Jun 8, 2020

If you feeling offended, it doesn't mean you are right. This is a total nonsense.

@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 8, 2020

I am personally not offended, but I am ashamed.

@Seldaek
Copy link
Member

Seldaek commented Jun 8, 2020

We are not looking for a debate here, if you disagree with this PR you can just close the browser tab and move on with your day.

@GrahamCampbell I understand fully that the internals need fixing, and as I said we already did a bunch of it in master branch, I just want to avoid needless conflicts because that's extra work for me, and the 1.10 branch is set to die out anyway. Anyway as you did it already whatever I guess just leave the PR as is, I'll check if it's a mess when trying to merge back into master.

@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 8, 2020

@Seldaek I am happy to help with the process of doing a 1.10 -> master merge after this PR is merged to 1.10. Since 1.10 is slowing down now, I don't think there will be much of a hassle with merge conflicts after that initial merge upwards. :)

@Seldaek
Copy link
Member

Seldaek commented Jun 16, 2020

As predicted a fuckton of conflicts, but it's merged in 1.10 and master (which already fixed all of this in slightly different ways, hence the conflicts..).

@Seldaek Seldaek merged commit da572f1 into composer:1.10 Jun 16, 2020
@GrahamCampbell GrahamCampbell deleted the allow-list branch Jun 16, 2020
@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 16, 2020

🎉

@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 16, 2020

Looking at the merge, Installer.php was not touched, but:

image

@Seldaek
Copy link
Member

Seldaek commented Jun 17, 2020

Right yeah that was already outdated before, and kinda meaningless anyway, fixed in e5fe35d

@brandonkelly
Copy link
Contributor

brandonkelly commented Jun 19, 2020

Thanks for doing this @GrahamCampbell !

@Big-Shark
Copy link

Big-Shark commented Jun 22, 2020

We are not looking for a debate here, if you disagree with this PR you can just close the browser tab and move on with your day.

Democracy and freedom of speech are not welcome here.

@brandonkelly
Copy link
Contributor

brandonkelly commented Jun 22, 2020

Composer is MIT licensed software so you certainly have the freedom to fork it and make whatever changes you want

@Oxonium
Copy link

Oxonium commented Jun 22, 2020

Great decision !

@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 22, 2020

I'm not being sarcastic, but actually what made you think there was a democracy here, or absolute free speech. If there was, there would be no need for moderation or a code of conduct, and everyone would have write access to the repo...

@composer composer locked as resolved and limited conversation to collaborators Jun 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants