🇨🇭Switzerland @luksak

@thulenb I'm happy to review MRs and opinions regarding my comment in #9.

Ok, I applied the changes that I could find. Since I couldn't find an official list of current tax rates, I hope I got it right... Please double-check that when reviewing.

In the MR the same changes are applied as in ✨ Adopt new Slovakia VAT rates starting from Jan 1, 2025 Active

Here is more detailed information about the tax rates. But I couldn't find a list of the current tax rates.

Also the 9% reduced rate is still in place: https://www.emta.ee/en/business-client/taxes-and-payment/value-added-tax...

I created a follow-up for 2.x: 📌 Adopt new Slovakia VAT rates starting from Jan 1, 2025 Active

luksak → created an issue.

I see the cahnces of someone being affected by the BC very low. But I want to communicate them. Should I do that in the relese notes of the next release?

Looks great thank you!

The patch works in some cases, but I have the same issues as described in #132

What's the status here can we get this commited?

Works for me too.

For anyone looking for a solution to the issues with the translation of rates and labels of the plugin, please refer to the follow-up here: ✨ Make shipping plugin fields translations configurable independently Active

The operators didn't work for me either. I ended up trying to write a custom condition that negates the default one commerce provides. Which turned out to be very easy:

<?php

namespace Drupal\MODULE\Plugin\Commerce\Condition;

use Drupal\commerce_product\Plugin\Commerce\Condition\OrderItemProductCategory;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;

/**
 * Provides the product category negated condition for order items.
 *
 * @CommerceCondition(
 *   id = "order_item_product_category_negated",
 *   label = @Translation("Product category negated"),
 *   display_label = @Translation("Product categories negated"),
 *   category = @Translation("Products"),
 *   entity_type = "commerce_order_item",
 * )
 */
class OrderItemProductCategoryNegated extends OrderItemProductCategory implements ContainerFactoryPluginInterface {
  /**
   * {@inheritdoc}
   */
  public function evaluate(EntityInterface $entity) {
    return !parent::evaluate($entity);
  }

}

Maybe we could even use that for this issue?

Ok, the last commit adds a lazy builder. This improved performance a lot.

Updating the wishlist block via ajax was accidentally removed in this MR. I just added it back.

Besides that the MR work perfectly for me.

luksak → made their first commit to this issue’s fork.

I added another MR that provides compatibility with ✨ Option to proivde remove links Active because the two MRs play nicely together. It simply updates the add to wishlist links.

@anas_maw You can send messages using MessageCommand. It works, but I guess there must be a better way to collect and display messages in ajax command than this:

$messages = \Drupal::messenger()->messagesByType('status');
foreach ($messages as $message) {
  $response->addCommand(new MessageCommand($message));
}
\Drupal::messenger()->deleteByType('status');

But as it is this just adds a message to the top of the page for me and doesn't really help the user to understand what happened.

I implemented a new cache context. I don't have a lot of experience with caching, so probably this could be solved in a better performing way. My approach potentially creates a lot of cache data. I'd happy to get a review on this.

I'm trying to look into the lazy builder, since that'd probably improve performance a lot.

Ok, now a remove link is being displayed if any (not the default) product variation is in the wishlist.

Ok, I guess this is why the cache context isn't working:

https://git.drupalcode.org/project/commerce_wishlist/-/blob/8.x-3.x/src/...

public function getContext() {
  return implode(':', $this->wishlistProvider->getWishlistIds($this->account));
}

I guess that we'd need a new context that takes into all wishlist items into account. WishlistCacheContext only takes wishlist entites into account.

@anas_maw Thank you for your feedback!

Yeah, I found the caching issue as well. But your approach simply disables caching altogether. I think we could do better.

I tried to use the wishlist cache context. This improves the situation, but doesn't solve it completely. The cache somehow doesn't vary when a product is added to the wishlist. Any ideas how to solve that? Do we need a different context?

Additionally we might want to use a lazy builder to improve performance.

luksak → changed the visibility of the branch 3481746-option-to-proivde to hidden.

luksak → created an issue.

I needed this for wishlist links as well and updated the MR with it. It works quite well for me.

Shouldn't we try to use a data-attribute selector for the count? The approach with the class won't work for most sites... But yeah, that'd require commerce and commerce_wishlist to change the templates. Alternatively we could make the selector configurable? Or we'd need to let users know that the functionality depends on that selector and that users should add it to their templates in case they want it to work.

But at least the limitation of using a <span> for the count could be resolved.

Any other takes on this?

luksak → made their first commit to this issue’s fork.

I took a first stab at this.

There is an incompatibility with commerce_stock_enforcement. There are errors when clicking one of the actions in the success dialog. The happen because the form state is missing, but we're still extending the AddToCartForm. Here is the error:

TypeError: commerce_stock_enforcement_get_context(): Argument #1 ($entity) must be of type Drupal\commerce\PurchasableEntityInterface, null given, called in /app/web/modules/contrib/commerce_stock/modules/enforcement/commerce_stock_enforcement.module on line 160 in commerce_stock_enforcement_get_context() (line 317 of modules/contrib/commerce_stock/modules/enforcement/commerce_stock_enforcement.module).

So I tried to still call parent::buildForm(), but hide the elements. That works for me so far.

I haven't looked into the "Out of stock" button behaviour of commerce_stock so far. I'm using a quite customized add to cart form.

luksak → made their first commit to this issue’s fork.

luksak → created an issue.

This is a duplicate of ✨ Provide option to only add item to wishlist once. Needs review

Ok, got it. Since I don't plan to fix this: Feel free to create an MR. I'm happy to review.

Closing since there was not feedback on this.

Closing this, since this works as designed.

Looks good to me. Please open issues in case you experience bugs with Drupal 11.

Actually... Shouldn't this go into config translation? The current solution uses string translation.

Thank you, merged the MR.

jsacksick → credited Lukas von Blarer → .

Lukas von Blarer → created an issue.

I'm sorry, The cause of my errors was something else. Still, this should be fixed I guess.

I needed a quick fix and simpy made all mentioned parameters required. This fixes the requests for me. But this probably should be fixed differently.

Lukas von Blarer → created an issue.

Ok, here is a re-roll.

The module does provide this option. You can find it at /admin/config/services/simplenews/settings/subscription. The is a checkbox with the label "Skip email verification (discouraged)".

The MR is a hacky fix for this.

Here is a first draft to fix this issue.

I was quite confused by this. I think the simplenews 4.x release took the wrong path here... This is a very unintuitive API. And in my opinion very prone to errors. Take a look at this:

<?php
  /**
   * Checks if subscriber confirmation should be skipped.
   *
   * @return bool
   *   TRUE if confirmation should be skipped.
   */
  public static function skipConfirmation() {
    // Skip if logged in or if configured to skip.
    return \Drupal::currentUser()->id() || \Drupal::config('simplenews.settings')->get('subscription.skip_verification');
  }
?>

Any logged in user can skip confirmation to subscribe any email address. And this is on an API level.

Lukas von Blarer → made their first commit to this issue’s fork.

Ok, found the issue. I chose the wrong name in the schema.

The fieldsets are being exposed properly for me.

I took a look at this. I think it is almost done, but somehow there is something missing. I get this error when visiting the explorer:

GraphQL\Error\Error: Type "WebformElementWebformFieldset" not found in document. in GraphQL\Utils\BuildSchema::GraphQL\Utils\{closure}() (line 149 of /app/vendor/webonyx/graphql-php/src/Utils/BuildSchema.php).

GraphQL\Utils\ASTDefinitionBuilder->internalBuildType() (Line: 190)
GraphQL\Utils\ASTDefinitionBuilder->buildType() (Line: 193)
GraphQL\Utils\BuildSchema::GraphQL\Utils\{closure}() (Line: 350)
GraphQL\Type\Schema->loadType() (Line: 326)
GraphQL\Type\Schema->getType() (Line: 548)
GraphQL\Utils\SchemaExtender::extend() (Line: 356)
Drupal\graphql_core_schema\Plugin\GraphQL\Schema\CoreComposableSchema->getFullSchemaDocumentOverride() (Line: 314)
Drupal\graphql_core_schema\Plugin\GraphQL\Schema\CoreComposableSchema->getSchema() (Line: 253)
Drupal\graphql\Entity\Server->configuration() (Line: 199)
Drupal\graphql\Entity\Server->executeOperation() (Line: 27)
Drupal\graphql\GraphQL\Utility\Introspection->introspect() (Line: 87)
Drupal\graphql\Controller\ExplorerController->viewExplorer()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 58)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 704)
Drupal\Core\DrupalKernel->handle() (Line: 19)

Lukas von Blarer → created an issue.

Lukas von Blarer → created an issue.

The MR works for me with simplenews 4.x.

Ok, that makes sense. No, I have no idea how to approach this.

Do you know a workaround for the issue I described though?

Lukas von Blarer → created an issue.

Ok, this works as designed. This is the solution to this problem: 🐛 Views - Anonymous User Fixed

I was able to make the code a bit more generic:

/**
 * Implements hook_ENTITY_TYPE_access().
 */
function HOOK_view_access(View $view, $operation, $account) {
  $route_name = \Drupal::routeMatch()->getRouteName();
  if ($operation === 'view' && str_starts_with($route_name, 'graphql.query')) {
    return AccessResult::allowedIf($view->getExecutable()->access('default', $account));
  }
  return AccessResult::neutral();
}

Maybe that should be documented in the docs here? https://graphql-core-schema.netlify.app/schema-extensions/views.html That's at least where I was looking for why this isn't working.

I'm having this issue as well. @Siegrist have you found a solution to this?

I have the same issue. @kaszarobert did you come up with a solution for this?

Lukas von Blarer → created an issue.

I think this doesn't solve the issue entirely. If I understand your approach correctly, this solves the incorrect order for non-company addresses. But the issue where the order is mixed up compared to the user input persists for company addresses. Is there a way to solve that too?

Lukas von Blarer → created an issue.

Oh, I just realized that using prefixes is the way the blacklist of purge_queuer_coretags works already. I don't think that that is optimal, since that gives us less flexibility, but I guess that's is another issue. Please ignore the patch in #13.

This one only checks for exact matches. But I didn't fix anything else. So just ignore this patch in case you don't need that.

Attached you can find a re-roll.

In my case I need exact matches. I want to blacklist node_list, but that would also match node_list:article, which is not what I want.

Lukas von Blarer → created an issue.

Lukas von Blarer → created an issue.

Lukas von Blarer → created an issue.

Lukas von Blarer → created an issue.

Lukas von Blarer → created an issue.

Lukas von Blarer → created an issue.

Lukas von Blarer → created an issue.

Right now I can't do a full review of the MR. Is there a way to get a diff of my module? That way I could reply to changes.

Hi there! Thank you for reaching out to me!

All fine for me that you ported my code here. Please make sure that I receive commit authorship for it. And I'd like to declare the sponsorship for it on the project page.

Lukas von Blarer → created an issue.

The current MR worked perfectly for a field formatter in a view. Thank you!

Sorry, probably that needs a better review ;)

This works like a charm! Thank you!

I think that for the specific use case of having an array with values this is the simpler and better solution than the approach of ✨ Add wrapper process plugin to wrap/unwrap values in arrays Needs review .

Sorry, I forgot to add the new files.