Change values of LanguageInterface::DIRECTION_(LTR/RTL) to ('ltr'/'rtl')

Honestly, the only time I've ever used this constant was when casting it to a bool, to know if RTL was "on" or not.
So this would make more work.

In Drupal/Core/Language/LanguageInterface, I changed it to:

DIRECTION_LTR = 'ltr'

and

DIRECTION_RTL = 'rtl'

.

Uploading to test. There will need to be more changes.

yeah, there will probably be default config to change

for example..
core/modules/language/config/install/language.entity.en.yml

id: en
label: English
direction: 0
weight: 0
locked: false
status: true
langcode: en

the value for direction there should be 'ltr'

(and maybe some places in code... maybe that are not using the const)

@tim.plunkett hm. can you remember where that was?
casting 1 to a bool would be true.. so it was like if(bool $direction) { // if rtl
should probably have been if ($direction == LanguageInterface::DIRECTION_RTL) anyway

It's in contrib (can't remember where!), not core. And yes, that's better code for sure, I was just stating my experience using it.

direction: 0

FWIW a month or so ago there was an issue to sweep all such magic numbers from core...

#2286403: Inconsistent use of magic number when constants DIRECTION_LTR and DIRECTION_RTL already defined.

There is always the change some have crept back in :)

I did

$ grep -R 'direction: 0' core/*
it gave

core/modules/language/config/install/language.entity.en.yml:direction: 0
core/modules/language/config/install/language.entity.und.yml:direction: 0
core/modules/language/config/install/language.entity.zxx.yml:direction: 0

In those files, I changed

direction: 0

to

direction: 'ltr'

I looked for direction: 1, however I couldn't find any.

I forgot to put single quotes around two of the ltrs, I fixed that.

Hi, @EllaTheHarpy

Can I suggest setting a value of LanguageInterface::DIRECTION_LTR, instead

so the line would look like

direction: LanguageInterface::DIRECTION_LTR,

you may have to add a line at the top like

use Drupal\core\language\languageInterface;

@martin107 we can't do that, since they are YAML files

updated the patch.
I think the current interdiff shows how this can be a cleanup, since most places just need the 'ltr' / 'rtl' strings anyway.

+++ b/core/modules/ckeditor/ckeditor.admin.inc
@@ -61,7 +60,7 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
-  $rtl = $language_direction === 'rtl' ? '_rtl' : '';
+  $rtl = $language_interface->direction === LanguageInterface::DIRECTION_RTL ? '_rtl' : '';

hm. another issue to make a const for _rtl? would have to actually read the code to tell.

----
so some of this is changes since we can now assume direction is always set, right?
If we are worried about changing that assumption here, we could just do

instead of

-  $attributes['dir'] = $language_interface->direction ? 'rtl' : 'ltr';
+  $attributes['dir'] = $language_interface->direction;

-  $attributes['dir'] = $language_interface->direction ? 'rtl' : 'ltr';
+  $attributes['dir'] = $language_interface->direction ? DIRECTION_RLT : DIRECTION_LTR;

This is green.

hm. another issue to make a const for _rtl? would have to actually read the code to tell.

I don't think so. It's being used in a render array as '#uri' => $button['image' . $rtl],. It's just some local variable.

+++ b/core/includes/theme.inc
@@ -1799,7 +1799,7 @@ function template_preprocess_page(&$variables) {
-  $variables['language']->dir     = $language_interface->direction ? 'rtl' : 'ltr';
+  $variables['language']->dir     = $language_interface->direction;

Just call the method in let's stop polluting this object with weird properties. Also considering what we are changing let's protect the property in both Language and ConfigurableLanguage and use the getDirection() method.

That hunk from theme.inc is not even required anymore :) This use case is now handled by DefaultHtmlFragmentRenderer.

Fixed the new 'direction: ' config values added in #2336743: When more than one language is added in the profile, the installer ignores those.

+++ b/core/includes/theme.inc
@@ -1799,7 +1799,6 @@ function template_preprocess_page(&$variables) {
-  $variables['language']->dir     = $language_interface->direction ? 'rtl' : 'ltr';

Was the only suspect out of this issue scope initially. But found(from git history) that it is not in use and suppose to be removed long time back.

+1 to RTBC.

Ok, let's get this in.

Discussed this with Alex, it's a small change to the YAML but won't have an impact on porting modules (unless they provide default language entities but that seems unlikely). Re-tagging D8 upgrade path/beta target.

Awesome patch!

The failures are the "Too many connections" type which mean,s testbot at the end of a long DRUPALCON has fallen over

back to rtbc.

Applying the rules outlined in #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? this change is bc breaking without the possibility of adding a bc layer. It is only a normal task - If this change was not rtbc'd at or worked on at Amsterdam we should not proceed but because it was, I think we can.

Probably spent more time per line of code talking about this patch than any other at DrupalCon Amsterdam when trying to figure out exactly what the implications of post-beta criteria were. Glad we found a way to apply the criteria consistently but also not punt this to 9.x due to the close review.

Committed/pushed to 8.0.x, thanks!