Wesel
Account created on 7 September 2011, over 13 years ago
  • Consultant at KRZN 
#

Merge Requests

More

Recent comments

🇩🇪Germany sunlix Wesel

Hey @mandclu,

thank you for your response.
Yes we are interpreting 'custom' as "The user choose what ever he want as end date/time".
In smart_date 4.1 with 'custom' no end date / time calculation was done and if 'custom' was the only value provided the 'duration' field was not displayed. This was our use case with it.

So yes, from my perspective, your interpretation is right. :-)

🇩🇪Germany sunlix Wesel

Hey nicocin,

thank you for accepting the MR. Would be happy to get the credit for this contribution. Thank you a lot. :-)

🇩🇪Germany sunlix Wesel

Hey nicocin,

thank you for accepting the MR. Would be happy to get the credit for this contribution. Thank you a lot. :-)

🇩🇪Germany sunlix Wesel

Found another code line where we can levearge the GuzzleClient for proper env settings to fetch the images.

🇩🇪Germany sunlix Wesel

sunlix changed the visibility of the branch 3498697-instagram-use-core to active.

🇩🇪Germany sunlix Wesel

sunlix changed the visibility of the branch 3498697-instagram-use-core to hidden.

🇩🇪Germany sunlix Wesel

I digged a little bit into it.

I think the main problem is that if custom is the only allowed value in the duration_increments you can't set custom for the default_duration. It will be casted to 0 because the config schema requires integer.

By that in the smart_date.js the setInitialDuration function hide the end date and end time field.
This hiding is intended for non custom values. But duration can't be initially set to custom due to the integer casting for default_duration

🇩🇪Germany sunlix Wesel

I think for external sources you can't leverage Drupal's core cache system like cache tags.
But we can set a max age value for the block or introduce a config to make an individual max age available to site builders.

I can make a MR if the community get a consens on the approach. Maybe we can get a maintainer feedback here?

🇩🇪Germany sunlix Wesel

@acbramley

do you still need a new 1.x release? Or are you fine for migrating to 2.x?
It should be a smooth migration if there a no real customizations.

🇩🇪Germany sunlix Wesel

Do you have any Browser-Console issues presenting after loading the page?

Did you had any kind of customizations via preprocess hooks or overloaded templates? Or just defaults?

🇩🇪Germany sunlix Wesel

@aurm

The update hook also uses to entityFieldUpdateManager to uninstall the field storage.
I think this part of the update hook have to altered to the database connection.

🇩🇪Germany sunlix Wesel

The only uncertainty I have is on the isset check.
Should I change this to:

before:
if (!isset($path_should_be_tracked))

after:
if (!isset($path_should_be_tracked[$path]))

I have researched on other contrib modules like paragraphs permissions.
There is a direct check on if (!isset($path_should_be_tracked[$path])) instead the general one.

🇩🇪Germany sunlix Wesel

Hmm this is my very first contact with drupal_static.
I have adjusted the code to store the boolean in the $path wise manner.

I hope anyone can review and help here out. I am unsure if I got it right. :)

🇩🇪Germany sunlix Wesel

I close this issue due to the release of 2.0.0 right now.
There we dropped the Drupal.behavior implementation because it is not needed for third party integrations like ReadSpeaker.

🇩🇪Germany sunlix Wesel

I close this issue due to the stable release of 2.0.0 right now.

The migration path is there for the Drupal internals except the need of checking the used template.

🇩🇪Germany sunlix Wesel

The docReader feature can be added in a new 2.x minor version.

🇩🇪Germany sunlix Wesel

I tested successfully a default migration from 1.x to 2.x.

Made some minor changes to the settings form so we are good to go I guess.

🇩🇪Germany sunlix Wesel

@Anybody
I should not have a high impact. The default Drupal js library behavior is already "load the scripts and the end of the <body>" So it is nearly at DOMContentLoaded. In default Drupal installation with olivero I get in both cases a Lighthouse Performance of 100%.
In comparision with matomo, they load their event tracking without defer too.

🇩🇪Germany sunlix Wesel

Okay got it. We extended the library definition of etracker/etracker.js if event tracking is enabled in the settings.
This adds the etracker.js to our library. But it is affected by the "force moving external scripts" settings from advagg.
The source of the problem is not the advagg setting but the more the interaction with all library definitions and the removal of the defer attribute, which is mandatory.

The solution

We split the library definition of the vendor script and our default event tracking into two seperate one with dependency definition.
So we can guarantee to script order will be stable and the script will work without defer

🇩🇪Germany sunlix Wesel

Hey there,

I just came back to this because I was thinking if this is outdated.

I applied all settings from the screenshot and now I can reproduce this error.
The error occurs by set the config "Move all external scripts to the top of the execution order".
If you disable this kind of forcing the script order no error occurs.

Do you have some insights why do you have this setting enabled and by what parameter a script is considered as "external"?
I try to understand in what way we could manage the script to be "unbreakable" if there is some external forces involved.

🇩🇪Germany sunlix Wesel

This is already fixed by 📌 Automated Drupal 11 compatibility fixes for etracker Fixed and part of the current 3.0 stable release.

Thank you!

🇩🇪Germany sunlix Wesel

I added that last trailing comma because I wanted to see the test green.
It feels unusual for me. But it is allowed: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Traili...

I will find my way dealing with this :D

Ready for a merge if no one disagree. :)

🇩🇪Germany sunlix Wesel

I did a manual test on the admin-js. The cookies related is covered by the fantastic functional test suite from @Grevil.
So I think this should be good. But we can leave it here for feedback from @Grevil.

If I lint the silly string in to one line eslint want to make it multiline because of the line length. :D
The only one we can do is to introduce constants for the long selectors.

🇩🇪Germany sunlix Wesel

Hey there,

as discussed latlely with @Anybody I have adjusted the suggested solution from @Grevil a bit.
It makes no sense to have a different behavior only if there is one language installed in the system.
So we streamline the returned array for all languages.

There as in general a lot of confusing stuff in there :D
The page title get the language code as a suffix if page title suffix not empty.
But that key is never set and we do not have any kind of configuration for that.

By time the whole .module hooks should get a meaningful refactorisation. :)

🇩🇪Germany sunlix Wesel

Finished some refactoring and solving the phpstan, phpcs hints.
eslint> is the only one with some complainings but from my point of view this looks like a false-positive from eslint.
These are all multiline querySelector calls and for me it makes no sense to a comma at the end of the parameter list, because there is no further parameters to be set.

Maybe @Grevil and @Anybody has a opinion on that?

/builds/project/etracker/web/modules/custom/etracker/js/etracker.admin.js
  19:59  error  Insert `,`  prettier/prettier
  47:62  error  Insert `,`  prettier/prettier
  58:54  error  Insert `,`  prettier/prettier
  74:80  error  Insert `,`  prettier/prettier
  81:90  error  Insert `,`  prettier/prettier
/builds/project/etracker/web/modules/custom/etracker/js/etracker.js
  61:75  error  Insert `,`  prettier/prettier
🇩🇪Germany sunlix Wesel

sunlix changed the visibility of the branch 3302882-review-and-fix to active.

🇩🇪Germany sunlix Wesel

sunlix changed the visibility of the branch 3302882-review-and-fix to hidden.

🇩🇪Germany sunlix Wesel

Hey together,

I have added a draft MR to review the block entity migration to the new block plugin and settings.
Can anyone review this to see if anything works as expected?

Any feedback would be awesome.

🇩🇪Germany sunlix Wesel

I am good with the state of the config schema.
This is merged to 2.x. I will release a alpha-verison to test that out.

Currently there is no migration path for the Block-Plugin changes (replaced with another plugin ID)
That will be a follow up on the road to stable.

🇩🇪Germany sunlix Wesel

@mcaddz

Thank you for your feedback. I have discussed this with our partnered agency and considered your response.
I am fully with you. Our assesment was similiar to yours.
We leave it as it is. I think there is no full separation of the user groups with the differrent accessibility needs.
So let them decide on their own.

But nevertheless, thank you for bringing that up.

🇩🇪Germany sunlix Wesel

I think the assertions in hook_options_list_alter are not enough to make it fail proof during runtime.

By documentation it should be only a debugging/testing feature.
https://www.php.net/manual/en/function.assert.php

I opened a merge request with early return's.

🇩🇪Germany sunlix Wesel

Ah nope, I think it's worng. I am sorry. aria-disabled does not change focusability. But it must excluded.

🇩🇪Germany sunlix Wesel

Maybe aria-disabled="true" is what we are looking for in this case?
aria-disabled="true" only indicates semantically the state, but it's not by function.
This would have to manually controled for custom elements but not needed here.

Do you have an opinion on this?

🇩🇪Germany sunlix Wesel

Thank you for your investigation.
I agree with the aria-hidden due to the redundant content argument.
But disabled is not an allowed attribute for anchor elements. So I think we should go with tabindex="-1".
But the current template has an accesskey="L". I think this could be a discrepancy or?

The original state of the template come direct from the vendor documentation.

🇩🇪Germany sunlix Wesel

We have re-add authorization_drupal_roles_entity_base_field_info for the migration update hook 8004.
In the update hook 8004 the base field is queried but it will not found due to missing metadata for the field API. (the database table is there, but not the field metadata).
So to make the migration the module need to know the old base field to make the query and fetch the data for the migration.
The module have to make the migration and in a later major version it can safely removed.

🇩🇪Germany sunlix Wesel

Hey marty2081,

thank you for this hint. I think this only applies for 1.x.

In 2.x you can use a hook_preprocess_block() to alter the drupalSettings

// OpenReadspeakerBlock.php
'#attached' => [
  'library' => $library,
  'drupalSettings' => [
    'open_readspeaker' => ['rsConf' => $config->get('rsConf')]
  ]
]

In 1.x you could also use the libraries-extend / libraries-override mechanic described here: Overriding and extending libraries

🇩🇪Germany sunlix Wesel

sunlix created an issue.

🇩🇪Germany sunlix Wesel

I have Cherry-Picked from 📌 deprecation dynamic property Fixed .
@cbccharlie was totally right, it's kind of a duplicate of 3384235. The solution does apply to 1.x Branch also.

For the GitLab CI there will be a dedicated issue. This is out of scope of this issue.

🇩🇪Germany sunlix Wesel

I have re-add the hook_entity_base_field_info to make the field authorization_drupal_roles_roles available again via Drupal::entityDefinitionUpdateManager.

🇩🇪Germany sunlix Wesel

Added a review for proposing null coalescing operator. So the change will be just 1 character. :-P
Thank you @pooja_sharma for your fast interaction on this!

🇩🇪Germany sunlix Wesel

Can confirm that the change works.
It's just the missing member variable for the dependency injection introduced in 📌 Preview all image styles based on FocalPointEffectBase Fixed :-)

🇩🇪Germany sunlix Wesel

I can confirm that this update works as intended. The error message in status report belonging to entity and field definition update disappears after that update-hook.

From my point of view this could be RTBC. A new release would be very helpful. :)

🇩🇪Germany sunlix Wesel

sunlix changed the visibility of the branch 3442694-typeerror-drupalschedulercontentmoderationintegrationeventsubscriberschedulereventsubscriberpublishimmediately to hidden.

🇩🇪Germany sunlix Wesel

Hey together,

this got my attention due to our addtion of this module in some customer projects.
I think this is already fixed by #3444174: Tests fail with "Call to a member function isPublished() on bool" and we just need a new release.

So I will close this as outdated and add a issue relation.

🇩🇪Germany sunlix Wesel

mea culpa, I did make an understanding failure. Thought it is defaulting to the ImageItem default settings, which is not, because here is no field api involved in the test szenario. :-)
Thank you for pointing that out, did learn something new with testing! :)
Should be good to go.

🇩🇪Germany sunlix Wesel

@pooja_sharma

Thank you a lot for pushing this forwars with a functional test. :-)
I have changed the test cases a bit to have to specific test case for given strings and a test case with no given alt attribute.
Both cases have to result in having a rendered alt attribute.

I am unsure if we have to add a specific alt = NULL testcase, because this is not intended by the used field API here. ImageItem have default values for it's properties which should not altered or?

🇩🇪Germany sunlix Wesel

Hey together,

I have research a little bit on this and I am sure, that the patch in die issue fork solve this.
The related property here is thumbnail__alt (Database column in media_field_data which comes from the BaseFieldDefinition in the Media Entity itself.
This property/column stay's null if you upload an image to a media entity based on the image source plugin and left the alt field empty.
But the ImageItem field plugin already has a default value for alt which is an empty string.
see: https://git.drupalcode.org/issue/drupal-3319601/-/blob/3319601-media-ima...

For remote video media entities based on the oembed source plugin this issue doesn't come up because the default value is set on the updateThumbnail method in the Media entity.
see: https://git.drupalcode.org/issue/drupal-3319601/-/blob/3319601-media-ima...

So the issue is "only" related to media entities which are based on media source plugins that have $thumbnail_alt_metadata_attribute defined.

🇩🇪Germany sunlix Wesel

Hey,

we have the same behavior in our installations. New installation with drush site:install --existing-config are not successfull if we have a content type configured to a content_moderation workflow.
Beside this patch we have to apply the path from 🐛 BaseFieldOverride fails to take into account ContentEntityInterface::bundleFieldDefinitions() when invoking onFieldDefinitionUpdate() Needs work to be able to successfull install from existing config.
Maybe that could related to each other. The error message differs but I think it is some kind of the same source issue.

🇩🇪Germany sunlix Wesel

I can confirm this.
I had multiple phone calls from customer on this topic today. But unfortunatelly did not found a reason yet.

🇩🇪Germany sunlix Wesel

Hey,

I have made a first shot so solve this from here.
We could change to module weight to be after the metatag_extended_perms_field_widget_single_element_form_alter and change back the #access
I think we can't proactive disable the elements display?

Production build 0.71.5 2024