Account created on 2 February 2021, over 4 years ago
#

Merge Requests

More

Recent comments

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

Wrong target.

🇩🇪Germany Grevil

All green now! Please review!

🇩🇪Germany Grevil

Done, tests fixed here: 🐛 Fix PHPUnit tests Active .

🇩🇪Germany Grevil

Done, please review!

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

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!

🇩🇪Germany Grevil

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.

🇩🇪Germany Grevil

@anybody, why have you removed the PluralTranslatableMarkup return type?

The "response_status" condition plugin has the exact same return type.

🇩🇪Germany Grevil

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?

🇩🇪Germany Grevil

Ok nice! Adding the dependency in the info.yml is enough :)

All green! Merging.

🇩🇪Germany Grevil

Green now! Let me try something.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

Done, please review!

🇩🇪Germany Grevil

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.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

There was a problem with the issue fork. I pushed the changes directly to main.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

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.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

Ok, that should be it. The approach wasn't correct. Sorry for the gitlab noise.

🇩🇪Germany Grevil

All done, please review!

🇩🇪Germany Grevil

This should be it.

🇩🇪Germany Grevil

It is in general weird, that the default options differs from defineOptions to defaultExposeOptions. That is not the case in the parent class.

🇩🇪Germany Grevil

Ok, edited my other comment. Should be all fine now.

🇩🇪Germany Grevil

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.

🇩🇪Germany Grevil

Postponed until Add a token for the site logo Needs work is even merged.

🇩🇪Germany Grevil

Created the follow-up issue here: 📌 Add alt text configuration for [site:logo] token Postponed .

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

I removed the alt text, replaced the deprecated "theme_get_setting" call and rebased the issue fork.

🇩🇪Germany Grevil

All done, please review!

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

Thank you, @rkoller! Back to "Needs work" to implement the recommended usability adjustments.

🇩🇪Germany Grevil

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?

🇩🇪Germany Grevil

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.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

Setting to RTBC for now, but still needs feedback by @smustgrave concerning #14.

🇩🇪Germany Grevil

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.

🇩🇪Germany Grevil

Right, reverted again.

LGTM then!

🇩🇪Germany Grevil

Ok, that should be it.

🇩🇪Germany Grevil

grevil changed the visibility of the branch 3438199-automated-drupal-11 to hidden.

🇩🇪Germany Grevil

Only dropped support for Drupal 9. Otherwise, this LGTM!

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

But not in its current state.

🇩🇪Germany Grevil

That should be it!

Please review!

🇩🇪Germany Grevil

Otherwise, LGTM!

🇩🇪Germany Grevil

grevil changed the visibility of the branch 3435463-automated-drupal-11 to hidden.

🇩🇪Germany Grevil

We shouldn't support Drupal versions, which reached their EOL.

🇩🇪Germany Grevil

That should be it. Please review.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

grevil changed the visibility of the branch 3069964-argument-1-passed to hidden.

🇩🇪Germany Grevil

Current MR as static patch.

🇩🇪Germany Grevil

[...] 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!

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

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!

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

🇩🇪Germany Grevil

LGTM!

🇩🇪Germany Grevil

Only makes sense. LGTM!

🇩🇪Germany Grevil

> This test did not perform any assertions weird....

🇩🇪Germany Grevil

LGTM!

🇩🇪Germany Grevil

Perfect.

🇩🇪Germany Grevil

Added one comment. Otherwise LGTM!

🇩🇪Germany Grevil

Yes, all green again.

🇩🇪Germany Grevil

Sorry, forgot to message the expected deprecation messages inside the tests. Should be green now!

🇩🇪Germany Grevil

@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.

🇩🇪Germany Grevil

Still, static patch for the time being.

🇩🇪Germany Grevil

Static patch for the time being.

🇩🇪Germany Grevil

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.

🇩🇪Germany Grevil

IMO, the current MR should simply target 2.x, unfortunately I don't have the right permissions.

🇩🇪Germany Grevil

IMO, the current MR should simply target 2.x, unfortunately I don't have the right permissions.

🇩🇪Germany Grevil

That's it.

🇩🇪Germany Grevil

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.

🇩🇪Germany Grevil

@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.

🇩🇪Germany Grevil

LGTM!

🇩🇪Germany Grevil

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.

🇩🇪Germany Grevil

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!

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

@dev is already D11 compatible.

@kongkx please create a new release!

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

grevil created an issue.

🇩🇪Germany Grevil

Ok, found one more D11 incompatibility issue. Also, some weird "negate" behavior, but this should be fixed in a follow-up issue.

Please review!

🇩🇪Germany Grevil

Funny thing is, dev is already d11 compatible: https://git.drupalcode.org/project/jsonapi_role_access/-/commit/512c00af....

But there never was a release.

🇩🇪Germany Grevil

grevil made their first commit to this issue’s fork.

Production build 0.71.5 2024