Porta Westfalica
Account created on 9 May 2008, about 17 years ago
  • Drupal Software Engineer & Developer, Project Management at DROWL.de 
#

Merge Requests

More

Recent comments

🇩🇪Germany Anybody Porta Westfalica

Confirming we just had this error on the following path:
https://www.example.com/user/register?_wrapper_format=drupal_ajax&ajax_f...

Backtrace:

TypeError: mb_strlen(): Argument #1 ($string) must be of type string, array given in mb_strlen() (line 332 of /web/core/lib/Drupal/Core/Form/FormValidator.php).
#0 /web/core/lib/Drupal/Core/Form/FormValidator.php(332): mb_strlen()
#1 /web/core/lib/Drupal/Core/Form/FormValidator.php(246): Drupal\Core\Form\FormValidator->performRequiredValidation()
#2 /web/core/lib/Drupal/Core/Form/FormValidator.php(238): Drupal\Core\Form\FormValidator->doValidateForm()
#3 /web/core/lib/Drupal/Core/Form/FormValidator.php(238): Drupal\Core\Form\FormValidator->doValidateForm()
#4 /web/core/lib/Drupal/Core/Form/FormValidator.php(118): Drupal\Core\Form\FormValidator->doValidateForm()
#5 /web/core/lib/Drupal/Core/Form/FormBuilder.php(593): Drupal\Core\Form\FormValidator->validateForm()
#6 /web/core/lib/Drupal/Core/Form/FormBuilder.php(326): Drupal\Core\Form\FormBuilder->processForm()
#7 /web/core/lib/Drupal/Core/Controller/FormController.php(73): Drupal\Core\Form\FormBuilder->buildForm()
#8 /web/core/modules/layout_builder/src/Controller/LayoutBuilderHtmlEntityFormController.php(39): Drupal\Core\Controller\FormController->getContentResult()
#9 [internal function]: Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult()
#10 /web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array()
#11 /web/core/lib/Drupal/Core/Render/Renderer..php(638): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#12 /web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(121): Drupal\Core\Render\Renderer->executeInRenderContext()
#13 /web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext()
#14 /vendor/symfony/http-kernel/HttpKernel.php(181): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#15 /vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw()
#16 /web/core/lib/Drupal/Core/StackMiddleware/Session.php(53): Symfony\Component\HttpKernel\HttpKernel->handle()
#17 /web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle()
#18 /web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php(28): Drupal\Core\StackMiddleware\KernelPreHandle->handle()
#19 /web/core/modules/big_pipe/src/StackMiddleware/ContentLength.php(32): Drupal\Core\StackMiddleware\ContentLength->handle()
#20 /web/core/modules/page_cache/src/StackMiddleware/PageCache.php(116): Drupal\big_pipe\StackMiddleware\ContentLength->handle()
#21 /web/core/modules/page_cache/src/StackMiddleware/PageCache.php(90): Drupal\page_cache\StackMiddleware\PageCache->pass()
#22 /web/core/modules/ban/src/BanMiddleware.php(50): Drupal\page_cache\StackMiddleware\PageCache->handle()
#23 /web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\ban\BanMiddleware->handle()
#24 /web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle()
#25 /web/core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(36): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle()
#26 /web/modules/contrib/remove_http_headers/src/StackMiddleware/RemoveHttpHeadersMiddleware.php(49): Drupal\Core\StackMiddleware\AjaxPageState->handle()
#27 /web/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\remove_http_headers\StackMiddleware\RemoveHttpHeadersMiddleware->handle()
#28 /web/core/lib/Drupal/Core/DrupalKernel.php(741): Drupal\Core\StackMiddleware\StackedHttpKernel->handle()
#29 /web/index.php(19): Drupal\Core\DrupalKernel->handle()
#30 {main}

No idea if it was a hacking attempt or what caused this, but the issue is real.

🇩🇪Germany Anybody Porta Westfalica

Just wondered, why GA (via google_tag) was not blocked on an older project using klaro and the reason was, that this update was missng.

Maybe Klaro should add a mechanism to update the default configs to ensure the relevant entries are up to date? (And better they exist than they do not?)

TL;DR: Voting for update hooks in such cases to add the relevant / new entries, if not yet existing. Should we open a follow-up for that?

After adding the new entries, Klaro blocked correctly, but this is a bit dangerous for "normal" users?

🇩🇪Germany Anybody Porta Westfalica

@maintainers: Any chance to merge this finally?

See: https://git.drupalcode.org/project/klaro/-/blob/3.x/README.md

For Google Tag 2.x you may consider to disable the noscript snippet (see #3106318).

🇩🇪Germany Anybody Porta Westfalica

Thanks for the super helpful and important investigations @grevil!

I think we should do two things:
a) handle the different link types (rel's: (information_link, payer-action, redirect, ...?) as described in 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active in Print details from error messages returned by paypal Active so that if user action is needed, it works!
b) Try to reduce the cases where a user action is needed by default, e.g. might mean to include a default shipping (country) rate in the first call in the cart

🇩🇪Germany Anybody Porta Westfalica

Thanks @grevil. I'm not sure if we should track the point in time, when the order was paid in full as an additional base field or instead should calculate it based on the tracked payments (IF $order->isPaid() is true).

Let's wait for maintainers feedback on how they'd say it's best.

Which way ever we go, we need to ensure that the information is always consistent, for example if a payment was refunded afterwards.

🇩🇪Germany Anybody Porta Westfalica

anybody created an issue.

🇩🇪Germany Anybody Porta Westfalica

Great work for a great feature based on the work done by @vmarchuk! Thanks for the various important finding and fixes!

@vmarchuk will you or your team have a look soon so we don't lose focus here? Would be wonderful to have this feature released soon IMHO!

🇩🇪Germany Anybody Porta Westfalica

Works as expected in my testing! RTBC!

🇩🇪Germany Anybody Porta Westfalica

Here we go, sorry for the code style fixes in the same commit. -.-

🇩🇪Germany Anybody Porta Westfalica

@grevil: I think the logical mistake is, that only product line items should be included in the JSON generally and never adjustments. I didn't look into the code, but hopefully it's easy to find the reason how VAT comes in here.

🇩🇪Germany Anybody Porta Westfalica

Thank you so much @jsacksick and @vmarchuk! We're on it!

🇩🇪Germany Anybody Porta Westfalica

And let's please close all fixed subissues then.

🇩🇪Germany Anybody Porta Westfalica

Guess this makes sense as next step, but we should first ensure we have a valid concept.

🇩🇪Germany Anybody Porta Westfalica

@derekw is this maybe fixed in the meta issue already? 📌 [META] 11.x feature updates Active
Could you priovide a MR otherwise perhaps?

🇩🇪Germany Anybody Porta Westfalica

@grevil this is ready so far, please review by time and autofix issues. I think we'll surely find some points for @lrwebks left to resolve next week.

🇩🇪Germany Anybody Porta Westfalica

Let's do this as follow-up. @lrwebks has a list of possible Scheduler issues that might help with the implementation.

🇩🇪Germany Anybody Porta Westfalica

@tomtech any chance to merge this? Would be helpful and is a nice improvement I think?

🇩🇪Germany Anybody Porta Westfalica

@vmarchuk the testing environment is currently broken, could you revive it?
TypeError: Drupal\Core\Entity\Sql\DefaultTableMapping::Drupal\Core\Entity\Sql\{closure}(): Argument #1 ($definition) must be of type Drupal\Core\Field\FieldStorageDefinitionInterface, __PHP_Incomplete_Class given in Drupal\Core\Entity\Sql\DefaultTableMapping::Drupal\Core\Entity\Sql\{closure}() (line 175 of core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php).

So we could proceed the comparison. Would it be possible to share a screenshot of your Drupal Stripe payment gateway settings? (admin/commerce/config/payment-gateways/manage/stripe_payment_element)

🇩🇪Germany Anybody Porta Westfalica

@tomtech plans to merge this into 2.x? As paypal is quite common...

🇩🇪Germany Anybody Porta Westfalica

Sorry, this wasn't the reason for our trouble. I had to use single_use for expected payment methods to appear.

🇩🇪Germany Anybody Porta Westfalica

@damienmckenna thanks for your reply. In my eyes both is true.

Currently the issue is, that theres a "Live" vs. "Sandbox" switch, but you can only store public / private key for one of them. This combination doesn't make much sense in our eyes.
When switching the radio, you have to exchange the keys and enter the opposite ones and when switching back, same again...

The goal here is to add separate fields & config values for live and sandbox credentials, so you can switch at any time, if you need to, by just switching the radio.

Of course it should also be documented how to set these values in settings.php.

🇩🇪Germany Anybody Porta Westfalica

Yeah, I had similar thoughts.

I like:

  • PayPalErrorHandler
  • PayPalErrorResponseHandler
  • PaymentExceptionHandler
  • PaymentErrorHandler

PayPalErrorHandler or PayPalErrorResponseHandler are most clear and descriptive IMHO.

Anyway the service gives us wonderful options for future extensions and changes. Nice work @Grevil! :)

🇩🇪Germany Anybody Porta Westfalica

Okay I think we should first and finally resolve 🐛 Documentation improvements Active to improve documentation and add details from the reply here. Maybe close this as duplicate afterwards?

🇩🇪Germany Anybody Porta Westfalica

Sorry but I can't reproduce it. Any chance that there's a misconfiguration? Maybe wrong text format for the affected contents, where Glossify isn't enabled or configured wrongly?

🇩🇪Germany Anybody Porta Westfalica

@jsacksick sadly no, I think there are cases where Paypal asks for further verifications, see the linked issue:

Using PayPal Checkout with the buttons in cart can sometimes lead to a PayPal error with
name: UNPROCESSABLE_ENTITY
details: issue: PAYER_ACTION_REQUIRED

So PAYER_ACTION_REQUIRED is one of the possibly more complex examples. Refunds might be a simpler one.

🇩🇪Germany Anybody Porta Westfalica

An error handler service or helper would make sense if we were doing some mapping between a code and a human readable message which isn't the case here? We're just spitting the error returned right?

Nope, I think we need to do exactly that - handle messages differently based on their type and eventually perform additional actions, so there's some complexity.

For example, see here: 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active Paypal expects to (re)direct the user to a certain action URL.

🇩🇪Germany Anybody Porta Westfalica

@jsacksick thanks - yes I also think we should use the messenger for such details. What do you think about the general architecture - putting this into a dedicated service to handle the different types of messages etc.?

🇩🇪Germany Anybody Porta Westfalica

Just my 2 cents: I think handling the JSON returned from PayPal is essential, especially for cases, where buyer interaction is needed, like in 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active .

Putting the handling into a dedicated service also makes a lot of sense to me, to being able to centrally handle such messages and related actions. For example, showing the informational link or even redirecting users. Otherwise, such logics may mess up the code around.

🇩🇪Germany Anybody Porta Westfalica

Simple, but effective fix! :) Thanks!

🇩🇪Germany Anybody Porta Westfalica

I can confirm this. Input should definitely always be trimmed for this reason. As it causes a hard exception, I'd say this is high prio.

🇩🇪Germany Anybody Porta Westfalica

No idea yet, sorry. Sounds like some kind of race condition / timing issue...

🇩🇪Germany Anybody Porta Westfalica

Please provide a proper description by filling the issue template:

🇩🇪Germany Anybody Porta Westfalica

I can confirm this issue, a client entered the YouTube video URL without www. and that lead to oEmbed error message "No matching provider found."

🐛 Remove YouTube videos in library cause "No matching provider found." Active

🇩🇪Germany Anybody Porta Westfalica

Suggestion implemented in MR, please review. Is it fine to place the variable in the Exception or do we need any kind of escaping? (I don't think so). Without these details, the message will not be helpful to show in any way (in my opinion).

🇩🇪Germany Anybody Porta Westfalica

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

🇩🇪Germany Anybody Porta Westfalica

3.x is unsupported, what's the situation in 5.x? Can we close this outdated?

🇩🇪Germany Anybody Porta Westfalica

@rkoller thanks, yes that's because this condition plugin doesn't seem to be applied on blocks, if I remember correctly.
We ran into this in contrib modules that allow all condition plugins, e.g. https://www.drupal.org/project/posthog or https://www.drupal.org/project/context - unsure if there's also a place in core where it's shown.

@Grevil can you please update the IS accordingly by time, if I'm right?

🇩🇪Germany Anybody Porta Westfalica

Yeah maybe one day... if anyone does it. We won't implement this ourselves.

🇩🇪Germany Anybody Porta Westfalica

@thomas.frobieter core lazy loading is already supported in photoswipe now. Using the lazy module is quite untypical since lazy has native support, but if anyone is interested to write that as contrib module, we're happy to link that.

🇩🇪Germany Anybody Porta Westfalica

Thanks @grevil totally makes sense and code LGTM so far. Sadly, I have no clue, how an update hook for a condition plugin can be written. I'm also unaware of any existing examples, while I think they possibly exist.

Is there maybe any core maintainer or someone else experienced with that? We'd really like to finish this, for UX and bugfixing reasons, as currently it's kind of broken by design ;)

🇩🇪Germany Anybody Porta Westfalica

LGTM, just one question for @thomas.frobieter - and ready for his testing and CSS implementation.

🇩🇪Germany Anybody Porta Westfalica

Whao this is crazy good @grevil! Thank you! LGTM, let's wait for @grimreaper's feedback!

🇩🇪Germany Anybody Porta Westfalica

Ok here are the new settings as discussed with @thomas.frobieter, to implement as checkboxes.

Hide controls [hide]
Show vidstack controls [show_vidstack]
Show vidstack controls (on hover) [show_vidstack_hover]
Show native controls [show_native]
🇩🇪Germany Anybody Porta Westfalica

From my opiniton this would be a perfect example for a settings.php setting in core which can be overridden... but let's see what the others think.

🇩🇪Germany Anybody Porta Westfalica

@grevil thanks, that looks exactly as planned. Regrading the entity question: I thinkt here must be a way to get all entity types / bundles that use a certain field type, here: entity_access_password_password

Those should then be processed, I think. Do you agree?

🇩🇪Germany Anybody Porta Westfalica

Yeah in general this is just my 2 cents. We should not be too strict with this, it won't harm too much if it's not perfect yet.

🇩🇪Germany Anybody Porta Westfalica

Thanks for the detailed feedback @tomtech. Let me add some further points:
1. I indeed think that the current "Items have been added to the cart"-message is confusing on login and unexpected, meaning no good UX for the user
2. I agree that a clearer message would be helpful to let the user know that carts have been combined - but definitely shouldn't be the same as now. With a clear message, it would be a bonus, while the current message is a malus.
3. I agree that creating the message might better fit into commerce_combine_carts then, but that also means that the current message from commerce_add_to_cart_confirmation needs to be suppressed in that case, so probably it's an issue in both modules in combination then?

To sum things up from my perspective:

  • commerce_add_to_cart_confirmation
  • Needs more information about the context from where items have been added to cart
  • Should not show the confirmation in all of these contexts

commerce_combine_carts

  • Needs to provide one of these contexts
  • Might show a different message, informing the users that carts have been merged on login. That could also be a regular Drupal "Success" message

So from my perspective, it looks like most of the essential things still need to be resolved in commerce_add_to_cart_confirmation code-wise and logically?

🇩🇪Germany Anybody Porta Westfalica

Great work! All green now, let's merge this.

🇩🇪Germany Anybody Porta Westfalica

@droath I now opted the module into security advisory policy. I'll wait before committing other fixes to this module to hopefully get in contact with you. Anyway, I'll always respect your work as original maintainer and hope we can help you maintaining the module.

🇩🇪Germany Anybody Porta Westfalica

@grevil thanks, LGTM, let's wait for further feedback.

🇩🇪Germany Anybody Porta Westfalica

@lazzyvn no, this is not specific to admin theme, it's specific to paragraphs_table. We can't assume other containers should be horizontally scrollable.

I understand your special relationship with bootstrap 5 theme, but this in a very helpful module for anyone, also Gin users and other Drupal users. Please consisder that. And currently this is neither working for Gin nor for Claro, without implementing this. People should never be forced to use your bootstrap 5 theme to have an acceptable UX. In general modules should not rely on specific admin themes.

🇩🇪Germany Anybody Porta Westfalica

Yeah big +1 on this. Currently a wide table kills the form displays. Definitely the wrapper should be scrollable. We should ask @thomas.frobieter for a MR.

🇩🇪Germany Anybody Porta Westfalica

Thanks! A new release would be great!

🇩🇪Germany Anybody Porta Westfalica

Thanks @thomas.frobieter great work! I think that's totally fine. RTBC from my side! :) Really nice improvement, we should suggest this for core link fields widgets as follow-up.

🇩🇪Germany Anybody Porta Westfalica

Sorry, stupid me... wrong project! Should have been on https://drupal-mrn.dev/

🇩🇪Germany Anybody Porta Westfalica

Thanks @grevil! Yes I think we should unset it, at least in the UI. In the backend we could eventually map it...

🇩🇪Germany Anybody Porta Westfalica

@lrwebks: Ok let's do it like this, as just discussed with @thomas.frobieter:

Replace the current controls checkbox with a "Controls" select with the following options:
- Hide controls
- Show native controls
- Show vidstack controls

()Maybe in the future we'll have further ones like "Show XX controls on hover"...

This needs no update hook, as the module is not used anywhere yet.

🇩🇪Germany Anybody Porta Westfalica

We just got feedback from Billwerk that this bug is now fixed on their side. I tried with removed workaround and everything seems to work well now, so I'll remove the workaround then.

🇩🇪Germany Anybody Porta Westfalica

Before implementing anything here, I think we need to come to a consensus what's expected and what should be handled (in the UI) or if this is essentially a vidstack bug.

I think when using Vidstack, we always want to use the Vidstack controls and I'd expect Vidstack to hide the native controls. Adding a further checkbox would delegate this confusion to the user, so I'd vote to either report this as bug to Vidstack and see what they say or (at least for now) set the attributes in a way that isn't confusing, so the user only controls the vidstack controls and if they are OFF, other controls are also hidden.

🇩🇪Germany Anybody Porta Westfalica

Yes, I'd also prefer A as a nice improvement. B could be an additional option using a select like we for example know from field groups? Maybe as follow-up?

🇩🇪Germany Anybody Porta Westfalica

When solving this, either the formatter should get a "Use widget setting / default" option or the widget should generally override the formatter setting and the formatter should have a description informing about that like "Notice: If the widget allows overriding, this setting will have no effect."

🇩🇪Germany Anybody Porta Westfalica

I agree, but as nobody complained so far, we should wait for a MR to review, if someone needs it.

🇩🇪Germany Anybody Porta Westfalica

Nice work @Grevil. Some final additions and please check code-style. The complaints look weird to me, but we should ensure code style is fine! :)

🇩🇪Germany Anybody Porta Westfalica

Nice finding @jan kellermann that looks promising and totally makes sense! Thank you for the super quick reply!

🇩🇪Germany Anybody Porta Westfalica

@thomas.frobieter I just had the idea to contact @chi, maintainer of https://www.drupal.org/project/twig_tweak and ask if we could maybe implement a merging solution that respects a certain field display formatter for drupal_field(). What do you think about that?

Production build 0.71.5 2024