Ok, I am now using the "starterkit_theme" instead of the "test_theme", since its label is wrapped in strong tags for some reason.
Also adjusted the summary logic slightly, to support the old deprecated "theme" config key.
Please review once again!
Note, that test failures are unrelated. The tests are simply not compatible with the newest PHPUnit version. We should fix that in a seperate issue.
@anybody, why have you removed the PluralTranslatableMarkup return type?
The "response_status" condition plugin has the exact same return type.
Ok, I created the MR and made the code a bit prettier. Please review!
I am a bit unsure about the output array:
$elements[$delta] = [
'#markup' => $video_url->toString(),
'#url' => $video_url,
];
looks a bit odd don't you think?
Ok nice! Adding the dependency in the info.yml is enough :)
All green! Merging.
Should work as expected, from "ProductAvailabilityDefaultFormatter":
// If the min / max delivery period is empty, we should use the global
// fallback:
if ($minDeliveryPeriod === NULL) {
$minDeliveryPeriod = $this->globalSettings->get('min_delivery_period_fallback');
}
if ($maxDeliveryPeriod === NULL) {
$maxDeliveryPeriod = $this->globalSettings->get('max_delivery_period_fallback');
}
I need to take a deeper look.
There was a problem with the issue fork. I pushed the changes directly to main.
Ok, I really don't get this whole switch case, or maybe I am missing something.
In line 110 we are already assigning the route parameters to the params:
// Massage the route parameters using the tour route parameters.
$params = $route_parameters;
Meaning, for the added test here, we already have "taxonomy_term: 1" now additionally we add in the switch case 'bundle' = 1 or now with the fix 'id' = 1. Both is unnecessary, since $tour->hasMatchingRoute will only check on "taxonomy_term: 1" the other added array values are completely ignored. That's why both tests succeed here, with and without the TourHelper adjustment. We could delete the WHOLE switch case and the test would still succeed, since the $params = $route_parameters before the switch case is enough, for this case to work.
Still changing the status to "Needs review" as at least it now fixes the incorrect "bundle= 1" addition.
Ok, that should be it. The approach wasn't correct. Sorry for the gitlab noise.
It is in general weird, that the default options differs from defineOptions to defaultExposeOptions. That is not the case in the parent class.
The description is referring to a boolean value which is kind of taxing for less technical users and should probably be avoided . And in combination with the label of the negate checkbox it makes things hard to comprehend. the cognitive load is rather high.
Adjusted the description accordingly. It now uses a similiar description to the "language" condition plugin.
add the menu link summary for consistency reasons.
This one is trickier than I thought. I have no idea, where this is defined in the condition plugin. I looked at other plugins but still no idea...
Interestingly enough, the "Vocabulary" condition has the same problem:
In general the preferable reading/processing order on those conditional plugins would be first what you want to do (aka the negate checkbox or the radio button pattern) and after that "what you want to apply it to"
This should be fixed in a general issue for all conditional plugins, since ALL of the use the same pattern:
Applied first
Negate second
you simply can not see it, since Blockform disables negation for all other condition plugins in line 253-265 of "BlockForm":
// Disable negation for specific conditions.
$disable_negation = [
'entity_bundle:node',
'language',
'response_status',
'user_role',
];
foreach ($disable_negation as $condition) {
if (isset($form[$condition])) {
$form[$condition]['negate']['#type'] = 'value';
$form[$condition]['negate']['#value'] = $form[$condition]['negate']['#default_value'];
}
}
IF it would be technical possible try to use a radio button component instead of the negate checkbox.
Same here, we should create a central follow-up issue for this as well. Since the "negate" behavior is baked in code through "ConditionPluginBase" "isNegated()" method:
/**
* {@inheritdoc}
*/
public function isNegated() {
return !empty($this->configuration['negate']);
}
This is not covered in the MR / Patches of [#14640540]. Therefore, I'd leave it as is for now.
I hope this is ok as is. Please review.
Postponed until ✨ Add a token for the site logo Needs work is even merged.
Created the follow-up issue here: 📌 Add alt text configuration for [site:logo] token Postponed .
I removed the alt text, replaced the deprecated "theme_get_setting" call and rebased the issue fork.
Thank you, @rkoller! Back to "Needs work" to implement the recommended usability adjustments.
I think I found the problem. Since the minification and build process is done through vite, there is no actual "Drupal.t()" in the processed tour.js anymore and the Drupal t regex won't match, which results in the string not being translated.
I tried turning off the minifaction in the vite config, but this results in further console errors AND the function becomes "Drupal2.t()" which could also still be a problem.
Unsure how to continue here, since we established, that we don't want to use the "normal" Drupal approach for the js library here: https://www.drupal.org/project/tour/issues/3548238#comment-16282931 🐛 Broken when using JS aggregation Fixed .
@smustgrave, what do you think? Any idea, besides refactoring the JS library to conform to Drupal standards?
I added the config_translation.yml. Everything looks ok, but I can not seem to get the Next / Previous Buttons translated, no matter what. I already translated "Next" and "Previous" in the User Interface translation, cleared all caches and ran cron several times to check, if there are new / multiple "Next" and "Previous" elements on the translation page, but no.
I also rebuilt the tour.js and added console.logs for the `tourStepConfig` to check if "Drupal.tour.nextButton" and "Drupal.tour.previousButton" works as expected, but everything looks fine. Seems like Drupal.t doesn't work in that context for some reason.
We should investigate this further.
Setting to RTBC for now, but still needs feedback by @smustgrave concerning #14.
Perfect, looks great! RTBC!
Added one comment, but unsure whether it is out of scope or not, but I don't like the new label. We should revert it and adjust it in a potential follow-up issue, OR fix it here and adjust the label as mentioned in the MR.
grevil → changed the visibility of the branch 3438199-automated-drupal-11 to hidden.
grevil → changed the visibility of the branch 3435463-automated-drupal-11 to hidden.
We shouldn't support Drupal versions, which reached their EOL.
[...] For the kind of bug that can be seen in the code, I (personally) think a commit would be more helpful than waiting for tests forever
Not yet forever, but 4 years are close enough! Added patch #4 as an MR. Please merge @tedbow, so we don't wait 4 more years for this!
Made some adjustments to the info.yml, removed the composer.json (as it doesn't contain module specific dependencies and will get auto-generated) and removed some unnecessary code in the settings form.
Please review!
Ok, https://git.drupalcode.org/project/watchdog_mailer/-/commit/f8c53ca1cdb7... should fix the issue.
Sorry, forgot to message the expected deprecation messages inside the tests. Should be green now!
@joelwinzer can we work on the MR please and only use patches as static representations of the MR's state? Also, why would you expand on an old patch???
@arthur.baghdasar this probably happens because the patch you mentioned expands on an old version of this issue's fix.
Ok I rebased everything and reverted https://git.drupalcode.org/project/commerce_paypal/-/merge_requests/45/d... for the time being.
"payer-action" is definitely still handled incorrectly here, but if we process the links, these links should be handled properly as well...
Setting back to "Needs work" the goal of this issue is unclear, and the implementation is flawed.
IMO, the current MR should simply target 2.x, unfortunately I don't have the right permissions.
IMO, the current MR should simply target 2.x, unfortunately I don't have the right permissions.
I agree with @anybody here. IMO, no alt text at all isn't good either.
We have a similar case in core, where the Drupal logo isn't perfect @core/modules/system/src/Plugin/Block/SystemBrandingBlock.php:
$build['site_logo'] = [
'#theme' => 'image',
'#uri' => theme_get_setting('logo.url'),
'#alt' => $this->t('Home'),
'#access' => $this->configuration['use_site_logo'],
];
I'd say we leave it as is and create a follow-up issue, where we add the ability to change the alt text for both the Branding Block and this token.
@wigglykoala, please open a dedicated issue for this.
You can fix this issue through properly installing the photoswipe caption library (recommended to require via composer: Enable use of third party libraries:
https://www.drupal.org/docs/develop/using-composer/manage-dependencies#t... →
and run composer require npm-asset/photoswipe-dynamic-caption-plugin:^1.
But of course this should NOT throw an error.
Since we haven't heard anything from @rkoller since #55 🐛 Drupal Current theme condition plugin should provide an option to select all themes Needs review , I guess we can remove the "Needs usability review" tag?
The only place this is used is in block, and there it is unset, because it is completely unusable in its current state.
Alright, that should be it!
- I added further comments inside the tests for the deprecated "theme" key, so once it is not supported anymore, the reason for the failing test can be found quite easily.
- I updated the deprecation messages to note about the removal in Drupal 13 (instead of 12).
- I updated the change record to reflect this.
Please review!
@dev is already D11 compatible.
@kongkx please create a new release!
Ok, found one more D11 incompatibility issue. Also, some weird "negate" behavior, but this should be fixed in a follow-up issue.
Please review!
Funny thing is, dev is already d11 compatible: https://git.drupalcode.org/project/jsonapi_role_access/-/commit/512c00af....
But there never was a release.