🇩🇪Germany @Anybody

PHPUnit tests are failing

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

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

Could this please be turned into a MR?
Also please consider ✨ Redirect user after login to a specific page Needs review

@paul_leclerc I'd be fine with that, but could you please add this as MR?

Could this please be turned into a MR? @siemen_hermans?

@shobhit_juyal could you please provide this as MR?

@agentrickard plans to provide a MR to solve this? I'm not using this module much, just became maintainer because there were no active maintainers and I wanted to ensure compatibility with latest D11 versions.

As this is marked as Major, I think it should definitely be resolved.

Do we care about the potential security risk of JS leaking during the redirect process?

I also don't see a high risk here, but we introduce some kind of new bug then anyway. Perhaps there's a way to solve both cleanly?

Thanks @godotislate, sorry I didn't see that. Fixed that. As both were only textual changes, I'm setting this back to RTBC, but feel free to confirm that.

Great work @jan kellermann thank you!!

Thank you @grevil :)

joachim namyslo → credited anybody → .

See #31

Thanks, please retry afterwards!

@heddn, so it actually makes sense now, to merge #3494209: Drupal 7 to Drupal 11 migration runs forever and this issue!

Merge now and proceed here also fixing that issue? (To simplify testng).

Important note is: The other issue is still unsolved and we have not found the root cause yet.

Leaving for @heddn to decide how to proceed. We could also copy the D11 changes from here over to the other issue for now to keep them separate for now...

@phenaproxima I agree with @heddn's comment, what do you think? Would be great to get this fixed, looking at the stack of modules waiting for this to be fixed. For that reason setting priority to Major.

First I think we now need to agree on how to proceed.

Thanks for the fixes @grevil! Hopefully entity_embed will become D11 compatible soon...

@grevil No I think there's a pattern to solve this. Look into similar widely used modules, they surely had the same issue.
Separate releases are no good solution for such a specific issue.

anybody → changed the visibility of the branch 3416508-only-file-javascript to hidden.

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

Confirming RTBC, but DI would be the preferred way. Leaving this for the maintainer to decide.

Thinking about this a bit I think it can't use DI as the call causing this is a static call?

@saidatom could you provide a stack trace maybe to see where this comes from?

Thanks, since this was already committed, I think we can close this fixes.

@eleonel please see this slightly related critical D11 issue: 🐛 Using a translatable string as a category for field type is deprecated Active

This breaks Drupal 11 "Add field" functionality entirely. See 🐛 Changes in site building process can run into Cannot access offset of type StringTranslation\TranslatableMarkup on field creation path Active

This should please be merged asap and a new release needs to be tagged. Thanks!

Root cause for us (we were able to find that thank to @berdir's MR): 🐛 Using a translatable string as a category for field type is deprecated Active

Update: The attached MR adding an exception with very helpful details GREAT!

Instead of the message in #20 we now see a nice verbose error:
Exception: Invalid field category for field typeentity_reference_display, category must be the ID of aa defined field category, got Referenz. in Drupal\field_ui\Form\FieldStorageAddForm->processFieldDefinitions() (line 136 of core/modules/field_ui/src/Form/FieldStorageAddForm.php). from which we can see it's a migration issue and how to fix this.

RTBC! Would be nice to cherry-pick this into 10.4.x and 10.5.x

We're running into a very similar issue in the function:

TypeError: Cannot access offset of type Drupal\Core\StringTranslation\TranslatableMarkup on array in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 45 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php). 

Drupal\Core\Plugin\DefaultPluginManager->getDefinition() (Line: 16)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance() (Line: 76)
Drupal\Component\Plugin\PluginManagerBase->createInstance() (Line: 136)
Drupal\field_ui\Form\FieldStorageAddForm->processFieldDefinitions() (Line: 80)
Drupal\field_ui\Form\FieldStorageAddForm->buildForm()
call_user_func_array() (Line: 528)
Drupal\Core\Form\FormBuilder->retrieveForm() (Line: 279)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
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: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 50)
Drupal\ban\BanMiddleware->handle() (Line: 96)
Drupal\crowdsec\Middleware->handle() (Line: 263)
Drupal\shield\ShieldMiddleware->bypass() (Line: 162)
Drupal\shield\ShieldMiddleware->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 49)
Drupal\remove_http_headers\StackMiddleware\RemoveHttpHeadersMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 709)
Drupal\Core\DrupalKernel->handle() (Line: 19)

I'll try to find the root cause now.

This indeed still fills up the logs and is confusing. This is nothing that should be at exception level, I think. For us, it means, that monitoring sees this as (kind of critical) error like the application is broken somewhere. We should really fix this, or at least reduce the severity for that reason. Especially because the site owner can't do anything about it?

@murat_kekic thank you very very much, this sounds promising. Having a test for this is most important. Very much looking forward.

Maybe something like this? (See MR)

🐛 Call to a member function getCurrencyCode() on null Active should also be applied (and merged before this one).

🐛 AddToCartEvent WSOD for order items with no price Active is similar, but not the same. Should be merged afterwards.

This is the related code:

public function getData(): array {
    // @todo leverage config for token replacement.
    $order_item = $this->getContextValue('item');
    assert($order_item instanceof OrderItemInterface);
    $unit_price = $order_item->getUnitPrice();
    assert($unit_price !== NULL);
    $adjusted_price = $order_item->getAdjustedUnitPrice();
    assert($adjusted_price !== NULL);

    $item_data = [
      'item_name' => $order_item->label(),
      'affiliation' => $order_item->getOrder()->getStore()->label(),
      'discount' => $unit_price->subtract($adjusted_price)->getNumber(),
      'price' => $this->formatPriceNumber($unit_price),
      'quantity' => (int) $order_item->getQuantity(),
    ];

assert($unit_price !== NULL); safes us in dev, but not in prod, I guess?

We maybe this needs more defensive checking also for production?
This is major for Drupal Commerce projects.

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

Please note that $item->getPrice() may also return NULL:
@return \Drupal\commerce_price\Price|null — The price, or NULL.

For that reason, I also changed

protected function formatPriceNumber(?Price $price): string {

to accept null values and return 0.00 then! Otherwise, I think me might run into the next issue, if $price is NULL somewhere? Hope I wasn't too defensive now.

Just saw that there are various other cases with similar calls. On it!

I can confirm the error and WSOD! Please merge this and tag a new release asap! Thank you!

I'll add a similar MR fixing code style and solving this a bit shorter. Maintainer may decide which approach is better, but this definitely needs to be more defensive!

@mradcliffe sound good, could you prepare a MR draft?

@SpadXIII are you sure this really speeds things up? Do you have any statistics for that?
Each PHP object is recreated, if it's not reused itself, so I guess in most cases this is not using the cached values?

Still it would make sense to implement better caching, especially if icons are used several times on a page.
I just think the implementation might not be the best possible solution?

Tests seem to be running now in 2.x? Guess this is an old one?

I think this is a duplicate of ✨ Create a micon + linkit (and micon + linkit + link_attributes) submodule Needs review

Happy to get support or sponsoring from the Drupal community. We're also a small company supporting this project in our spare time. Thank you!

FYI: I think defer would be the right choice logically and based on our experiences with COOKiES. Still if claro is loaded at the end of the page, the effect should be minimal.
See similar comments here: 🐛 COOKiES increases LCP a lot (Google pagespeed) Active

Thank you very very much @simonbaese! :)

Please recheck with latest version!

anybody → created an issue.

anybody → created an issue.

No I think that's a very good idea. phpcs and phpstan can be solved in a separate issue. Thank you!

Thanks @hudri. Yes these are separate topics. Using defer is generally fine for this lib, while it won't have large effects. Async would indeed be wrong.

I also dislike the LCP idea in Lighthouse and had similar thoughts.

Maybe changing the default lib_scroll_limit config setting to 1 pixel would be a sensible default for most users?

Yes I agree that might fix the LCP issue, not a bad idea! :) Any negative side effects? Cookies (non-required) are still blocked until the user scrolls and accepts them?

Anyway improving the default text is also a good idea.

The module addresses an issue in the upgrade path from Drupal 9 to Drupal 10. Is it possible to upgrade directly from 9 to 11?

Yeah that's the main purpose, but we had other cases where it is helpful to be able to remove orphaned permissions, for example the field permission module may create such cases. Same can happen for migrations from Drupal 6 or 7.

This module may not be relevant for Drupal 11. I'm not sure if we should bump this. Can you give your opinion?

Well I just had a case where I needed it in D11, why don't you think it's relevant? I think there can still be orphaned permissions?

Let's add a generic test and see if it's green!

Drupal 11 is out

block_component_library.install is unacceptable this way, please look yourself.

anybody → created an issue.

The attached MR does NOT fix the root cause, but maybe we should handle this better? Unsure...

This is the affected line:
https://git.drupalcode.org/project/svg_image/-/blob/3.x/modules/svg_imag...

Looks like no fallback image style is present? Maybe a misconfiguration?

@jan kellermann sorry we're busy on projects that need to get finished this week, I can't promise I'll be able to review it this year, but it's on our radar. Thanks for your impressive work!

anybody → created an issue.

anybody → created an issue.

anybody → created an issue.

Thanks, let's tag alpha1!

Follow-up: 📌 Write tests for the submodule and without persistent tracking Active

Thanks @grevil! All resolved! :)

Thank you @koustav_mondal! LGTM!

📌 Write tests for the submodule and without persistent tracking Active

anybody → created an issue.

Fixed in ✨ Add option to use other storage methods Active

Fixed in ✨ Add option to use other storage methods Active

Many alternative storage methods are now present in ✨ Add option to use other storage methods Active so I think this won't be needed. Otherwise happy to review a MR if anyone wants to.

Minor now that we have other methods than cookie. Maybe already works, we should prove that with tests in the submodule.

Fixed in ✨ Add option to use other storage methods Active