Jose Reyero → created an issue.
So... this is the patch, but still, since the module has a drupal/core dependency on composer.json, you won't be able to run the D10 core composer upgrade....
To get that, using this branch's MR....
1. Edit your composer.json file and add the repository, add the package to the exclude part in the main repository...
"repositories": [
{
"type": "composer",
"url": "https://packages.drupal.org/8",
"exclude": [ "drupal/commerce_sepa"]
},
{
"type": "git",
"url": "https://git.drupalcode.org/issue/commerce_sepa-3401525.git"
}
2. Then on the command line...
composer require drupal/commerce_sepa:dev-3401525-drupal-10-compatibility
Have fun, test the D10 upgrade and report back...
Here goes the patch
Jose Reyero → made their first commit to this issue’s fork.
Jose Reyero → created an issue.
This will fix the issue, just copied form other Quick Tabs plugin...
Jose Reyero → created an issue.
We've had the very same issue unrelated with migration and it looks like there's something wrong with how PaymentOrderUpdater
works.
The thing is you may only see this error on the console when using cron as it is triggered from PaymentOrderUpdater::destruct(), maybe it doesn't even get into the logs as it is produced when the request is being destroyed.
It happens when you delete a commerce order from some cron process. Then PaymentOrderUpdated keeps the order id cached, but fails to reload the order, that was deleted, when doing the clean up operations...
And yes, the patch fixes the issue, or at least prevents the error, which I'd say is a very bad one as it can go hidden for a long time and have very unexpected consequences, since it prevents the request from being properly terminated...
This patch should fix it.
Not sure whether the update could be simplified just using the configuration data... However I've kept the whole plug-in-settings + defaults update code just to make sure the resulting settings are exactly the same....
Jose Reyero → created an issue.
Thanks for the review @rkoller,
About 1, it would be really hard, to get the right order here because we are somehow blindly translating properties that have a real form somewhere else, but we cannot use that one... so not sure we can actually get that - order - information from anywhere..
About 2. Agreed, this form sucks... just like all the other config translation forms I guess :D ... anyway the difference here is we are doing some more targetted 'form altering' - to add field names, etc.. - so I'll see about not collapsing all the fieldsets as default...
The form being always small is not a criterium we can use though, as there may be many more fields or properties here, it will just depend on each content type configuration.. and each field type has its own story too.... I'll try with some 'count nested fields < x => do not collapse'..
Jose Reyero → made their first commit to this issue’s fork.
Well, looks interesting...
While I agree service decorators are a better option, adding a more generic decorator was not that easy for my case, it just gets too complex, https://www.drupal.org/project/3337204/issues/3337695 📌 Use Service Decorators instead of replacing TranslationManager Closed: won't fix
Anyway, I like this kind of decorators / wrappers you are proposing, one per source language, may be a good idea..
About the po exports, the idea is actually make one different PO per source language, so you can hand it over to a different translator... I need to work on this one, https://www.drupal.org/project/3337204/issues/3337265 🐛 Fix support for PO Export / Import Active
So I think you better just ignore my patch for now, and move on with MultilangNG and your new architecture, then I'll see about more options to make it pluggable here, which looks easier now with all these new services you are adding.
This patch is a simple and rough version of #40 / 4B: Just use default language for drush - and CLI - *always*
It will fix the issue for most people, while also keeping consistent string translations. Just keep in mind the language when running drush will be *consistent* but maybe not the one you like.... as it's always the site default language.
(Not intended for committing, of course, just setting to needs review to trigger testing and see what happens)
Thinking a bit more...
* There's no easy way to use language consistently through the API, different languages (current, configuration) are retrieved from the container everywhere... that's dependency injection I guess ...
* Inconsistencies happen at other levels too. String translation is not properly initialized with drush either.
// This one uses default language
drush php-eval "print t('User name');"
// This one triggers language negotiation and uses defined language.
drush php-eval "print t('User name', [], ['langcode'=> \Drupal::languageManager()->getCurrentLanguage()->getId()]);"
So yes, just aiming for consistency we need to fix the language initialization, not only config override language, bug locale language too.
And this is where everything happens, but not when running drush:
// class Drupal\language\EventSubscriber\LanguageRequestSubscriber
private function setLanguageOverrides() {
$this->negotiator->setCurrentUser($this->currentUser);
if ($this->languageManager instanceof ConfigurableLanguageManagerInterface) {
$this->languageManager->setNegotiator($this->negotiator);
$this->languageManager->setConfigOverrideLanguage($this->languageManager->getCurrentLanguage());
}
// After the language manager has initialized, set the default langcode for
// the string translations.
$langcode = $this->languageManager->getCurrentLanguage()->getId();
$this->translation->setDefaultLangcode($langcode);
}
I've done some more tests with this one and Drupal 10.1.x...
As @nanak #22 mentions, behavior is changed after cache rebuilding was fixed. But we can reproduce it with the setup in the issue description and forcing the field rebuilding with drush, try:
drush cr && drush php-eval "\Drupal::service('entity_field.manager')->getFieldDefinitions('node', 'article');"
Some more debugging shows also the already mentioned mismatch #35 in getFieldDefinitions(), when running with drush, while current language is the one set as custom language (de), the config override language is the default one (en). So we can see why the patch here, which fixes that mismatch works https://git.drupalcode.org/project/drupal/-/merge_requests/1361
However, I don't think that is the best place to switch configOverrideLanguage... or maybe just if we switch it on a method call, we better switch it back before returning, but not sure about this...
What I think we need to do is fixing the internal consistency of getFieldDefinitions() using current language bug calling other methods that get configuration that may not be using 'current language', but 'config override language' instead...
And as a general rule, if we want some consistent API, there shouldn't be any function that does language dependent caching without ensuring anyting it calls is using the same language...
Thanks @Akram Khan for cleaning up most of the patch.
This is a minor update which:
- Updates failing test, it was not checking for the right string, since latest versions of the patch add proper field names
- Fixes a minor naming issue. We are translating "widget settings", not "format settings" for Form displays. It also adds some better title formats - by using '%placeholders' instead.
Reading through this issue, just some thoughts for now...
1. It looks like we have two different issues here:
* Drush not triggering / respecting language negotiation / not settling on a language early enough
* Bad caching, not respecting language either or mixing config language and caching language.
2. Which language should Drush select? Not clear for me... is there a request / a URL / a language prefix ... ? Since this may quickly get really confusing I think we should aim at settling on a sensible default, also something that won't mess with deployments on different environments... which is I think the main use of drush... This should be IMO the Site Default language, and that's it. Easy and clear.
3. Whatever the final patch is, triggering language negotiation mid-request doesn't look like a good idea to me... So either we set it during initialization or we don't.... Again, 2) may be a sensible default.
4. So... I see two ways to fix this issue: A) Proper language / initialization with Drush to (whatever makes sense) or B) Fix caching, aka.. make config language consistent with caching language (whatever it is) or maybe skip persistent cache.
Looks great @beagaliana, nice work!
+1 for this one.
Forgot to set the status to Needs review...
New patch. Mostly implementing own ideas outlined in #209
Main changes:
- Moved most of the new code - and some of the existing one - to field_ui module. It simplifies a few things, like we don't need to check whether module is enabled or not.
- Naming Fields, Formatters and Widgets using field_ui_form_config_translation_form_alter().
It may be a bit rough yet, may need maybe less comments (?) but this is the working example...
Adding two screenshots, one from the view display, on from form display.
I'm still having this issue, no Beans... However, no intention of reopening the issue as it's not even this module's bug, but I'll leave some patch here for anyone who needs it...
It works, thanks.
Simple patch.
Jose Reyero → created an issue.
Hi, the idea looks great.
Maybe it could be a bit more user-friendly using ::validateForm() and form validation message, also a human-readable text instead of a raw exception message ?
Patch looks good to me, already pretty polished...
What I'm wondering about is: how "safe" is to force a default preload attribute value on sites that didn't have any?
I mean, if I had a page full of videos with no 'preload' attribute, maybe just an image placeholder, and suddenly I get that preload=metadata default.... will browsers just start preloading when they didn't before?
The point is: does it make sense to force a default value? or to force any value at all? Should we skip the update script until the user actually edits the formatter?
For background, I've found this pretty interesting post about how browsers behave, but note it is 10 years old, couldn't find and updated one... https://www.stevesouders.com/blog/2013/04/12/html5-video-preload/
Simple patch...
Jose Reyero → created an issue.
I've been giving a try to this one and I can say it works well.
The edit/translate UI may need some improvements though. I don't think this can be considered 'usable' without adding proper field / widget names everywhere.
Then, about the implementation, some notes:
1. I think this should be moved, as much as possible to the field_ui namespace. The feature is dependent on field_ui being active, isn't it?
Instead of core/modules/config_translation/src/ConfigTranslation/EntityDisplayMapper
maybe use core/modules/field_ui/src/ConfigTranslation/EntityDisplayMapper
just as core/modules/node/src/ConfigTranslation/NodeTypeMapper - which is the only other example I've found
2. Bit hackish, though I understand there's no easy way of doing this - setting proper names. However, relying on the element title is a no-no IMO.
+/**
+ * Implements hook_preprocess_HOOK() for details element templates.
+ */
+function config_translation_preprocess_details(&$variables) {
+ if (isset($variables['element']) && $variables['element']['#title'] == 'Field widget') {
+ $field = ucwords(str_replace('edit-', '', $variables['attributes']['id']));
+ $variables['title']['#markup'] = $field . ' field widget';
+ }
+}I mean, we are doing a core patch, right? So we don't really need to hack around. If we need one more hook, we add it, if we need better form element markers, we add them - in field_ui module.
The best option I can think of, so far, instead of adding more hooks is maybe adding some ConfigMapperInterface::massageForm() to the interface... though not sure about it, I'm working on some proof of concept, not there yet...
The other option would be maybe... but same, working on that, not yet there:
/**
* Implements hook_form_BASE_FORM_ID_alter().
*/
function field_ui_form_config_translation_form_alter(&$form, FormStateInterface $form_state) {
/** @var \Drupal\config_translation\ConfigMapperInterface $mapper **/
$mapper = $form_state->get('config_translation_mapper');
// This is_a(), just to avoid hard dependencies on other modules classes. We wouldn't need it if the mapper was in this module though.
if (is_a($mapper, 'Drupal\config_translation\ConfigTranslation\EntityDisplayMapper')) {
// @todo Add field / widget names, etc, etc...
}
}
As I said, working on some options ... still haven't figured it out yet... but what I see is the config_translation UI is pretty complex and limited. So anything that makes it easier/better I think will be welcomed...
Updated task description to account for the fact that when you switch default language back to English, nothing happens, existing config language is never updated.
I guess we need to make locale_config_batch_set_config_langcodes() behave the same for all languages and not to make an exception if default language is "en", or maybe keep track of the last language set for configuration, and whether the default langcode has been changed or not...
This patch adds "srclang" parameter to strings, while still keeping the 'Langcode' context. So it wil work with and without locale_extend module.
It also avoids the need to render markup, by injecting and using the 'string_translation' service, so it should be cleaner and more performant.
Added some tests borrowed from locale module, just to test this doesn't break existing API for now.
@alexpott
> is a +2 enough for a rtbc?
I'd really like someone else to give some more feedback here, maybe that RTBC. I mean it's been 5+ years discussing this one, it feels like while closing the original bug report, we are leaving a lot of issues unanswered.
Also I share some of @geek-merlin #107 concerns:
>Up to some days ago this was effectively a meta issue with a fruitful discussion of potential approaches. I missed the shift in the discussion, and i think it was wrong, because it buries a good conversation. And we do that too often.
I've given this a try but really, decorating the TranslationManager services is way harder than just overriding it... Too many extra methods and protected properties...
@alexpott
Thanks for creating the other tasks, I'll follow there..
Only, about your new test... so a module edits its own configuration on hook_modules_installed... and that makes its own translations get messed up... I mean LOL! ...
But OK, let's say it's a valid use case... It only makes an stronger case for #111 ... We should never run that locale_system_set_config_langcodes() upon module installs... and it would also pass this test ... :D
Anyway... this is a +1 from me, or maybe a +2 seen that it can also pass the trickiest install test ;)
More reasons for this patch @geek-merlin... by moving the locale_system_set_config_langcodes() into the LocaleConfigManager service, it will be much easier to knock it out using service overrides. (!)
Jose Reyero → created an issue.
Ok,
I agree @alexpott's patch, any of them, is an improvement over what we've got atm and it should fix the main issue on this task.
Code looks good, though I didn't have the time for thorough testing yet.
As a side note, and ok, for a new task, the workflow when changing default language is badly broken... just wondering whether to create a new task or there's one already. (?)
Jose Reyero → created an issue.
Yess to static calls!!
Already added,
and reorganized modules (t_es, t_de moved to submodule), see https://git.drupalcode.org/sandbox/reyero-3337204
use Drupal\locale_extend\Translate;
Translate::es("Hola");
or, as you like...
use Drupal\locale_extend\Translate as T;
T::es("Hola");
Thanks for the idea, I think this is starting to look good.. :)
For more syntactic sugar, see locale_extend_ns, I had it around, just added.
https://git.drupalcode.org/sandbox/reyero-3337204/-/blob/1.0.x/locale_ex...
// Anywhere in modules or classes.
use function Drupal\locale_extend\es\tr;
// Will translate from Spanish
tr("Hola");
Interesting.
Added this one for locale_extend, https://www.drupal.org/project/3337204/issues/3337265 🐛 Fix support for PO Export / Import Active
Jose Reyero → created an issue.
The idea looks good to me, we cannot wait for ever for Drupal core...
Some ideas:
- Make it modular. Blocking config updates from locale is one thing, translating configuration from other languages is a different one.
- About translating strings from other languages, I'd like you to give a try to my latest experiment, which I had in mind for some time... Locale Extend https://www.drupal.org/sandbox/reyero/3337204 →
- Have you tried this one? https://www.drupal.org/project/config_import_locale →
Also, this composable-inheritance looks like a really cool thing... though not sure it helps with DX and maintenance.. (?)
Well, the more I test this, and think about it... yes... installing new modules when your default language is not English is a terrifying experience !!! Stll worse when you are doing it while deploying to production... :(_
So reconsidering a little bit... I think there is only one time when we should run that locale_system_set_config_langcodes() and that is *after a site/profile install* ... and never triggered after single extra modules are installed. That should also fix your issue "to ensure that problems do not occur" @alexpott?
(I've also noticed what happens when first you switch default language... nothing yet... but, then you configure something else, then you install some new module... then... boom!... configuration translations kind of implode... :.oO )
So I'm wondering, what if:
* We fix this annoying thing with locale/config_translation re-trans-translating itself, that would be #94 (still preferred) or #104 (makes some sense, but also makes it way more complex)
* We also kick that locale_system_set_config_langcodes() function out of hook_modules_installed() to somewhere after profile install.
?
I mean would this make everybody happier? - Not 'happy', just 'happier'... ok?
And yes @geek-merlin #101 will fix this issue for some people now you've added a description to the module ;) thx
@geek-merlin
I like being right and wrong at the same time, sounds like quantum computing... :)
So yes, and no... :D .. We need to fix this issue ASAP, which I can understand is a major headache for users. And we need to get a cleaner config translation workflow so we developers can have some peace of mind with Drupal translation. Thinking about creating another issue like "Do not mess with the language of the default configuration" or similar... and maybe some copy & paste and linking of comments here will help..
So, about @alexpott patch:
* Ok, about flag description, not that important anyway, it's the flag itself that's some hack.
* Not so sure about changing also locale_config_batch, that makes the patch 10x more complex
I think that batch is invoked from more places, unlike locale_system_set_config_langcodes() which was called only on module/theme install
* I liked more the previous patch #96.. but not sure about what else this latest one fixes - besides saving a function call, but that two steps also made the logic cleaner... (?)
Please lets keep us focused here.
(And yes I am the first one guilty of trying to extend the scope of this thread rebuilding the whole config translation thing but...)
* If a simple solution like @alexpott #94 works and fixes this issue, we should go for that.
* I still want some better config translation @kristiaanvandeneynde but that is a complex and lengthy one, maybe for a new task.
* Loving some of the ideas in MultiLangNG module, @geek-merlin, you had it well hidden - lack of module description.... you'll see me around that one... :)
Hi @alexpott, this looks a pretty interesting cool fix...
The part I'm missing though is that isUpdatingFromLocale = TRUE ... "Whether or not configuration translations are being updated from locale"...
... because this is not actually updating configuration translations from locale, it's a module install and we are just setting the config language to the default one... so even if it is somehow working (yes, we are preventing the locale system from attempting to automatically update strings from that or do anything else with that configuration), maybe we should revisit the concept of that variable....
Not sure, but maybe just changing the description to "Whether or not configuration updates are being triggered from locale, either for saving a configuration translation of for setting the configuration language." would do... ?
Hi @kristiaanvandeneynde I basically agree with your idea #51 about keeping the default config in the language it comes - ok, let's assume English.... but also, maybe, for the future... #74
About the excelent diagram in @Gábor Hojtsy #81 just outlines the current situation / behavior, not a goal nor a solution in itself, it is just a must read to understand this issue.
So, if we can agree about keeping the default config in English, that would be a good starting point, there are a lot of pieces we have to adjust for that to happen and produce a solution:
- The initial configuration import, we should do no translation on that one. This looks like the easier one...
- The Config edit UI -which is all around the site needs to 'be aware' of that too, and should edit only the right language configuration, not the default one.
- This may require redefining how config / config_translation -which is a different module, may be enabled or not - work together.
- Then what happens with newly created config, i.e. a new field... ? How do we store that one, should we create translation right away, mark it somehow, etc.. ?
Then we have multiple site-building paths like 'what happens if' you start single language but not English, then add some more, then enable / disable config / interface translation, then change default language, etc... Whatever we do, in whatever order, it needs to produce some consistent state so my guess is that will need some harder coupling / requirements between modules - language / config translation / interface translation -
... Not that it happens now which is also a bug by itself but if we are working on this we should try to get it right at once...
So... those are a lot of things... that's why I insist we need to agree on some basic approach before wasting too much time on patches.