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

Remove all instances of master/slave terminology from core #4451

Closed
jackaponte opened this issue Jun 22, 2020 · 27 comments · Fixed by backdrop/backdrop#3892
Closed

Remove all instances of master/slave terminology from core #4451

jackaponte opened this issue Jun 22, 2020 · 27 comments · Fixed by backdrop/backdrop#3892

Comments

@jackaponte
Copy link

jackaponte commented Jun 22, 2020

As mentioned in the comments on #4441, in another effort to remove language that is evocative of or drawn from systemic racism, I propose that we remove all instances of "master" and "slave" terminology from Backdrop core. I would suggest using "primary" and "replica" instead, as Drupal did.


State of current PR:

Search for slave

> grep -R "slave" .
./core/includes/database/database.inc:  $legacy = $options['target'] == 'slave';
./core/includes/database/database.inc:  $legacy = $options['target'] == 'slave';
./core/includes/database/database.inc:  $legacy = $options['target'] == 'slave';
./core/includes/database/database.inc:  $legacy = $options['target'] == 'slave';
./core/includes/database/database.inc:  $legacy = $options['target'] == 'slave';
./core/includes/database/database.inc:function db_ignore_slave() {
./core/modules/system/system.module:  if (isset($_SESSION['ignore_slave_server'])) {
./core/modules/system/system.module:    $_SESSION['ignore_replica_server'] = $_SESSION['ignore_slave_server'];
./core/modules/system/system.module:    unset($_SESSION['ignore_slave_server']);

search for master

> grep -R "master" .
core/includes/common.inc:  // https://github.com/GerHobbelt/nicejson-php/blob/master/nicejson.php.
core/includes/file.mimetypes.inc:      58 => 'application/vnd.oasis.opendocument.text-master',
core/misc/ckeditor/CHANGES.md:  Please note that the [`tests/`](https://github.com/ckeditor/ckeditor4/tree/master/tests) directory which contains editor tests is not available in release packages. It can only be found in the development version of CKEditor on [GitHub](https://github.com/ckeditor/ckeditor4/).
core/misc/ckeditor_old/CHANGES.md:  Please note that the [`tests/`](https://github.com/ckeditor/ckeditor4/tree/master/tests) directory which contains editor tests is not available in release packages. It can only be found in the development version of CKEditor on [GitHub](https://github.com/ckeditor/ckeditor4/).
core/misc/jquery.timeentry.js: Licensed under the MIT (https://github.com/jquery/jquery/blob/master/MIT-LICENSE.txt) license.
core/modules/ckeditor/js/plugins/backdropimage/plugin.js:    // https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/uploadimage/plugin.js
core/modules/file/file.module:    case 'application/vnd.oasis.opendocument.text-master':
core/modules/layout/css/grid-float.css: * Licensed under MIT (https://github.com/twbs/bootstrap/blob/master/LICENSE)
core/modules/simpletest/tests/system_test.module:  // https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/ServerBag.php#L61
core/modules/views_ui/views_ui.install: * Rename the 'always show master' display setting to 'always show default'.
core/modules/views_ui/views_ui.install:  $value = $config->get('views_ui_show_master_display');
core/modules/views_ui/views_ui.install:  $config->clear('views_ui_show_master_display');
  • NEEDS TESTING WITH REPLICA DATABASE
@jackaponte
Copy link
Author

I can't figure out how to change the labels on this issue; was going for the "milestone candidate - bug" and "type - task" labels that Jen used on #4441!

@jackaponte
Copy link
Author

@jenlampton Any thoughts on how to move this ticket forward similarly to #4441?

@jenlampton
Copy link
Member

@jackaponte I was able to add the labels (though, I had to try three times, they didn't stick at first?) I'll also double check that you are a member of the "Documentation and Organization" team, so that you can add milestones and labels to all issues in this repo. This is supposed to happen automatically any time anyone leaves a comment, but the bot has been broken for years :(

@jenlampton
Copy link
Member

jenlampton commented Jun 29, 2020

Any thoughts on how to move this ticket forward similarly to #4441?

I think all we need is for someone to file a PR, so... backdrop/backdrop#3177

There are a TON of instances of Master in views, as that's the human-readable name of the default display. This PR attempts to change all the views that are created in .install files, or when installing default views from .json files (so that on a Fresh install, all displays would be named Default instead) but I did not attempt to update all the views that are in tests, or add an update for existing views.

I expect some tests will fail where they are comparing a view in the test to a view that was created by one of the two methods above.

@klonos
Copy link
Member

klonos commented Jun 29, 2020

Thanks @jenlampton 🙏 ...the PR LGTM 👍

I cannot speak for any instances not picked up by the PR though - I'm assuming that this was the result of a search/replace initially, so also assuming that we caught them all.

@jackaponte
Copy link
Author

Thanks for moving this forward @jenlampton and @klonos!

@jenlampton
Copy link
Member

I'm assuming that this was the result of a search/replace initially, so also assuming that we caught them all.

There are still some instances of "master" referring to views displays, and specific branches of various GitHub repositories (ckeditor, etc) but all instances of "slave" have been removed, with the exception of the backwards-compatible database handling, and a check for the old session variable $_SESSION['ignore_slave_server'] which should both be removed in 2.x.

@klonos
Copy link
Member

klonos commented Jul 13, 2020

Cool, let's get this in then. We can always iterate in the future if more instances need to be removed.

@quicksketch
Copy link
Member

I found some issues in the current implementation where "replica" was accidentally used in Views config values instead of "default"; that would cause a setting to not be respected.

I think some of @klonos's suggestions are also worthwhile and not too difficult. Feedback left in the PR: backdrop/backdrop#3177

@quicksketch quicksketch added this to the 1.25.2 milestone Jun 7, 2023
@quicksketch quicksketch modified the milestones: 1.25.2, 1.26.1 Sep 15, 2023
@quicksketch quicksketch modified the milestones: 1.26.1, 1.26.2 Oct 6, 2023
@izmeez
Copy link

izmeez commented Oct 9, 2023

It looks like this is pretty close although the PR is expired. It looks like the changes are essentially to replace master/slave with primary/replica or where appropriate default/replica and the one additional place is the settings.php file where it is just in a comment.

@quicksketch
Copy link
Member

I went ahead and closed backdrop/backdrop#3177 since it's the older PR and improvements have been made in the second PR by @herbdool.

I resolved the conflicts in backdrop/backdrop#3892 so this should be back to ready for review.

@herbdool
Copy link

Ready for review again. I added a couple words to cspell that it caught - since this PR affects little bits of lots of files, it's not surprising.

@quicksketch
Copy link
Member

This looks good to me. We need to update the @since line for the version of Backdrop we're targeting, but I can do that on merge.

I would expect this to be a 1.27.0 change, but it is entirely backwards-compatible, so we could do the next bugfix release. What do folks think?

@olafgrabienski
Copy link

What do folks think?

The former milestones were also bugfix releases, and I think that's okay. But there will be many translation updates, so a minor release seems more appropriate. Also, in a minor release the change will get more visibility.

@quicksketch
Copy link
Member

I read through the PR and there are actually only 3 changed translations, all of which are administrative-only:

This change in Contact module:
webmaster@example.com => admin@example.com

And these two strings in Views:

- This will make the query attempt to connect to a slave server if available.  If no slave server is defined or available, it will fall back to the default server.
+ This will make the query attempt to connect to a replica server if available. If no replica server is defined or available, it will fall back to the default server.
- t('Use slave server')
+ t('Use replica server')

There are quite a few changes from Master to Default, but all sites would already have that single word translated. So string-wise I don't think this is a big concern.

@olafgrabienski
Copy link

So string-wise I don't think this is a big concern.

I agree! (And thanks for looking into it.)

@quicksketch
Copy link
Member

I have merged this into 1.x and 1.26.x! Yay, this is great. Thanks to everyone in helping remove this language. The final solution is also backwards-compatible until 2.x, when the old settings keys will be removed.

@klonos
Copy link
Member

klonos commented Oct 13, 2023

... I'd like us to iteratively make changes with subsequent PRs rather than expecting to fix everything in one go ...

Follow-up issue and PR here: #6262

@quicksketch
Copy link
Member

I found a variable name typo that might cause undefined variable notices: #6338. Simple problem for which I filed a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment