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

Merge Requests

More

Recent comments

🇩🇪Germany Grevil

New changes LGTM, and make the module D9 compatible once again! Thanks, @ankitv18! :)

🇩🇪Germany Grevil

Created the "follow-up" issue here: 🐛 Field Type "isEmpty()" method logic does not sync with template logic Active . Wouldn't even declare this as a follow-up issue.

🇩🇪Germany Grevil

@anybody, agreed this isn't easily done, because the "isEmpty()" would theoretically need to know the currently set formatter settings (to see which property is enabled) to 100% ensure the isEmpty logic, but having a field type method depend on a formatter setting doesn't sound like a good idea! 😛

But yea I'll create a follow up issue for this.

LGTM!

🇩🇪Germany Grevil

I contacted @rdamjanov regarding this request.

🇩🇪Germany Grevil

Made some final adjustments. @rajeshreeputra are you able to merge issues as well? Or are we still waiting for @bohart? Should we contact him via mail?

🇩🇪Germany Grevil

The test failures seem unrelated, since tests have not run automatically for a while now.

🇩🇪Germany Grevil

Agreed! Let's see what @dave reid has to say about this.

🇩🇪Germany Grevil

I'd agree with that!

Furthermore, there seems to be no (correct) asset packagist composer package (other than the one created by @herved).

Let's see, what @dave reid has to say about this.

🇩🇪Germany Grevil

All green! Merging, thanks! :)

🇩🇪Germany Grevil

Works great! Thank you :)

🇩🇪Germany Grevil

Alright, should be fixed now! Thanks for pointing me in the right direction @ankitv18!

🇩🇪Germany Grevil

Thanks for the info @ankitv18! :)

I'll take a look and see, if I can fix it.

🇩🇪Germany Grevil

I think we should even drop the D9 compat and create a new 3.x branch (after we fixed the pipeline). What do you think @Anybody?

🇩🇪Germany Grevil

As stated in the other issue. Not needed:

I don't think it needs backwards compatibility. We do not opt into previous major tests (D9), meaning that the tests will never automatically run in a D9 environment, hence we do not need any backwards compatibility for "EntityReferenceFieldCreationTrait".

🇩🇪Germany Grevil

I don't think it needs backwards compatibility. We do not opt into previous major tests (D9), meaning that the tests will never automatically run in a D9 environment, hence we do not need any backwards compatibility for "EntityReferenceFieldCreationTrait".

🇩🇪Germany Grevil

Ok, I found the reason we can't seem to override anything through 🐛 Field (widget) descriptions are displayed wrong (should not be in .form-item__suffix) Needs work .

The child element used in Dimensions (and Measurement as well) has its own "form_element" theme wrapper. And because we are rendering the child element in a foreach, we basically add 3 div wrappers around each input, which results in the current look.

I tried to simply remove the child element theme wrapper inside the parent class, but this seems to not be easily done. I am also not quite sure, why "Number" (physical_number) has a form_element wrapper, as it seems to be used internally only and not as a standalone widget.

The only problem is, that if we would simply remove the "form_element" theme wrapper from the child element, we would:

  • A. Introduce a breaking change, as "physical_number" can't have a description nor label anymore
  • B. "physical_number" won't support "#field_suffix" anymore, so we would need another way on how to render the unit field suffix / the "x" between the input fields.

This is how it looks, when simply removing the child element theme wrapper and "User cannot modify the unit of measurement":

As you can see the measurement and "x" is missing, because "#field_suffix" isn't supported anymore by the child element.

🇩🇪Germany Grevil

Alright, unfortunately this isn't easily fixable.

It seems we can not (easily) remove the theme wrapper of the child "physical_number" form element in "physical_dimensions" / "physical_measurement" and removing the "form_element" wrapper inside the "Number" ("physical_number") class itself would:
A. Introduce a breaking change, as "physical_number" can't have a description nor label anymore
B. "physical_number" won't support "#field_suffix" anymore and a lot would break.

Taking a look at the before and after in #9 🐛 Field (widget) descriptions are displayed wrong (should not be in .form-item__suffix) Needs work I would say, that the original issue is fixed and any further work should go into a following issue (or rather 🐛 Dimensions render element / field widget styling lost when used with Claro Needs work ).

🇩🇪Germany Grevil

The Number form element ("physical_number") has their own "form_element" theme wrapper set. Since this form element is rendered in Measurement ("physical_measurement"), which now also defines a "form_element" theme wrapper we have two wrappers in place, which will result in a weird looking gab between the input field and the label / help text:

I'll see if I can fix this without simply removing the Number theme wrapper.

🇩🇪Germany Grevil

Dropped the "container" theme wrapper. Should be all done now! :)

🇩🇪Germany Grevil

Sry, deprecation helper was introduced in Drupal 10.. We could simply drop the D9 compatibility or use "version_compare". I'd say we drop the compatibility as D9 has reached its EOL long time ago.

🇩🇪Germany Grevil

Alright, as the old "3435760-automated-drupal-11" branch contained a LOT of unrelated (and unnecessary) code style changes, I created a new branch "3435760-drupal-11-compatibility-cleaned" containing only the D11 compatibility changes.

I also used version_compare instead of the DeprecationHelper, to keep the D9 compatibility. Please review!

🇩🇪Germany Grevil

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

🇩🇪Germany Grevil

The "previous major" fail is because Drupal 9 (RIP), didn't have the DeprecationHelper so we cannot have a version for D9 to D11 anyway.

Sorry, I didn't know this! Thought they backported it to D9, but hence it has reached its EOL, they probably just ditched the backport.

🇩🇪Germany Grevil

Make sure, that npm-asset/js_cookie points to the correct library!

Which one should be used here?

🇩🇪Germany Grevil

@ressa, that would probably be the best approach for the end user I agree! But we would need to manually update the js library once in a while.

Although I am quite confused, as to why the npm-asset package and the gihub latest github release are version-vise out of sync! (3.0.5 vs. 1.0.0), seems like they point to two different repositiories. I'll comment that in the parent issue.

🇩🇪Germany Grevil

Usually a simple "drush cr" after the update would be enough here, but we can simply clear the cache as well! LGTM.

🇩🇪Germany Grevil

Alright, that makes more sense! Please review!

🇩🇪Germany Grevil

Might make actually make more sense, if we don't even catch the error and let the update abort!

🇩🇪Germany Grevil

Alright, that should do the trick. Also tested the update hook. If the js_cookie module is present but not installed, it simply gets installed through the update_hook successfully.

If its not present, we get the following warnings on update:

🇩🇪Germany Grevil

@herved there is already npm-asset/js_cookie, which is usually used for third party libraries in Drupal. See https://www.drupal.org/docs/develop/using-composer/manage-dependencies#t... .

But I am a big fan how this is solved in Photoswipe using hook_requirements(), together with 2 different library definitions (one using the cdn script the other the local script) and then create a settings page where you can enable using cdn (turned off by default).

Created an issue here: 📌 Module violates GDPR compliance through requring the js_cookie library via cdn Active . I think we should leave this issue open and adjust the description, once the other issue is fixed.

🇩🇪Germany Grevil

I'll do so!

Didn't realize, that newly added dependency in the module composer.json will automatically download on update. I wonder if that is also the case if there is only the automatically created composer.json by drupal 🤔

🇩🇪Germany Grevil

@anybody I don't think that is needed. In 99.9% of all cases, they won't have the js_cookie module downloaded / required anyway. And since the need to require / download the module manually, they'll surely activate it as well right?

🇩🇪Germany Grevil

Seems weird to me, that changes like https://git.drupalcode.org/project/xmlsitemap/-/merge_requests/46/diffs#... would fix any tests?

But not really relevant, in the end, the maintainer needs to decide here 🙂

Only https://git.drupalcode.org/project/xmlsitemap/-/merge_requests/46#note_3... left. Afterwards, I can ping the maintainer.

🇩🇪Germany Grevil

Alright, did some final adjustments, please review!

🇩🇪Germany Grevil

Just had a quick chat on slack with @jsacksick if we should implement this feature. His answer:

Why not [...]

He also said:

But I'd consider this low prio TBH

I think this is particularly important for payment panes

But not sure how you can clarify this from the UI

I'd suggest we create a first MR and wait for feedback 👍

🇩🇪Germany Grevil

Sorry, seems I overlooked that, when merging 📌 The js-cookie library is deprecated Fixed .

I'll get on that ASAP.

🇩🇪Germany Grevil

Tests are green now.

But I am really unsure about this MR... There are LOADS of unrelated (and seemingly unnecessary) code style changes, which makes reviewing quite cumbersome. Found one more issue, which I commented.

Besides, 18 new phpcs issues were introduced, but the maintainer should decide here in the end.

🇩🇪Germany Grevil

Tests are currently failing for previous major.

🇩🇪Germany Grevil

Had a quick chat with @alexpott on slack, he suggests creating a test, which reproduces the issue so it is easier to get into the in's and out's.

🇩🇪Germany Grevil

I think you are right! I'll quickly do some testing. Although we probably need some further code added regarding the fallback for label:

    if (empty($info['label'])) {
      $info['label'] = str_replace('-', ' ', Unicode::ucfirst($attribute_formatted));
    }
🇩🇪Germany Grevil

New additions by @fgm are necessary, but will result in a fatal error for Drupal < 10.3. Use the DeprecationHelper instead.

🇩🇪Germany Grevil

DEV is already D11 compatible, I'll ping the maintainer to create a new release.

🇩🇪Germany Grevil

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

🇩🇪Germany Grevil

Changes LGTM! Thank you! :)

Just added a gitlab-ci.yml, so we can see, that the tests are still intact on remote (they succeed locally already).

🇩🇪Germany Grevil

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

🇩🇪Germany Grevil

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

🇩🇪Germany Grevil

I'd say we drop the D8 support, as it reached its EOL a long time ago. Otherwise, LGTM!

🇩🇪Germany Grevil

Great work @lrwebks! I added a few more comments, which should be resolved.

@smustgrave here you said, that changing "unblock" to "unban" might need a "usability sign off" should we instead just use "unblock" instead of "unban" everywhere else?

Instead of a mixture of "unban", "unblock" and "delete"?

🇩🇪Germany Grevil

I am not really a big fan of having the "Unban selected" button in the "actions" section:

I think having the button under the table feels "more organically" and more similiar to other drupal lists:

Furthermore, it would be nice if the button only appears if any ips are selected (using the states api).

🇩🇪Germany Grevil

Update: current version still works with the base install of fast404.

🇩🇪Germany Grevil

@graber feel free to add @Anybody and me as maintainers!

You can check our profile for references. Cheers!

🇩🇪Germany Grevil

Ok, ready for review!

I am quite unsure about this change... I think we should NOT merge it like this and instead simply drop the fast404 compatibility...
moduleHandler->moduleExists is a simply too hardware intensive call and we call it twice per request now...
This could easily lead to people abusing the module for DDoS attacks.

I'd say we tag https://git.drupalcode.org/project/perimeter/-/commit/b01aeb2be8ef6493cf... as the last fast404 compatible release (3.0.4 if that is possible??), deprecate 3.0.3 and create a new 3.1.0 release without the fast404 compatibility.

🇩🇪Germany Grevil

Setting to Major, as the hotfix will do the trick for now.

🇩🇪Germany Grevil

Crazy, that this doesn't come up on runtime but only when running `update.php` / `drush updb`, I would've never thought, that Drupal goes through all defined event calls when updating!

I'll definitely add a test for this!

🇩🇪Germany Grevil

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

🇩🇪Germany Grevil

Last release is quite a while ago. Any plans for a D11 release, or is it recommended requiring the dev version? I'd be fine with that, but the module page seems outdated if it only shows drupal 9 and 10 compatibility.

🇩🇪Germany Grevil

The provided MR is 40 commits behind. The logic doesn't seem to represent the current code. Also, I don't get why we clear the field groups in this MR here.

But yea, let's focus on fixing 🐛 Field groups are not copied when cloning display RTBC first.

🇩🇪Germany Grevil

Tests definied through the "Automated testing" use DrupalCI, which is deprecated. The prefered way to do testing is through the new(ish) gitlab-ci pipeline! https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...

Tests won't run on the main module page anymore without a .gitlab-ci.yml.

🇩🇪Germany Grevil

Also changes routes to require additional parameters.

All routes using the "view_mode" parameter, have a "view_mode" parameter set. I don't think we need to do anything else here?

theme suggestions based on the flaggable id seems overkill. I understand it's already there, but I don't think we need to add even more variations.

I talked to our theme developer, and he said that at least the first two additions add a lot of useful benefit!

Adjusted everything else. Please review, last changes!

🇩🇪Germany Grevil

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

Production build 0.71.5 2024