New changes LGTM, and make the module D9 compatible once again! Thanks, @ankitv18! :)
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.
grevil → created an issue.
Any furhter twig adjustments by @thomas.frobieter should go into 📌 Fully implement, adjust and review the "commerce-product-availability-product-availability-default" twig template Active .
All done, please review!
@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!
I contacted @rdamjanov regarding this request.
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?
The test failures seem unrelated, since tests have not run automatically for a while now.
LGTM!
Agreed! Let's see what @dave reid has to say about this.
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.
All green! Merging, thanks! :)
Works great! Thank you :)
Alright, should be fixed now! Thanks for pointing me in the right direction @ankitv18!
Thanks for the info @ankitv18! :)
I'll take a look and see, if I can fix it.
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?
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".
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".
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.
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 ).
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.
Found one more thing...
Dropped the "container" theme wrapper. Should be all done now! :)
All done.
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.
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!
grevil → changed the visibility of the branch 3435760-automated-drupal-11 to hidden.
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.
Alright! Reverted that change! Please review once again.
Make sure, that npm-asset/js_cookie points to the correct library!
Which one should be used here?
- https://github.com/js-cookie/js-cookie
- https://github.com/delfimov/JS-Cookie (this is the one where npm-asset/js_cookie points to)
@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.
Thanks for taking the time! :)
Usually a simple "drush cr" after the update would be enough here, but we can simply clear the cache as well! LGTM.
Alright, that makes more sense! Please review!
Might make actually make more sense, if we don't even catch the error and let the update abort!
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:
→
@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.
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 🤔
@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?
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.
Alright, did some final adjustments, please review!
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 👍
Done. Please review!
Sorry, seems I overlooked that, when merging 📌 The js-cookie library is deprecated Fixed .
I'll get on that ASAP.
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.
Tests are currently failing for previous major.
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.
@eswiderski please use the MR instead, or create a static patch from it if needed: https://git.drupalcode.org/project/menu_item_extras/-/merge_requests/36....
Aight!
Make sense 😅
That should do the trick.
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));
}
👍
That's it!
New additions by @fgm are necessary, but will result in a fatal error for Drupal < 10.3. Use the DeprecationHelper → instead.
DEV is already D11 compatible, I'll ping the maintainer to create a new release.
grevil → changed the visibility of the branch 3435751-automated-drupal-11 to hidden.
All green! Merging...
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).
The parent module is still not D11 compatible.
POSTPONED on 📌 Automated Drupal 11 compatibility fixes for fakeobjects Needs review .
grevil → changed the visibility of the branch 3429020-automated-drupal-11 to hidden.
I'd say we drop the D8 support, as it reached its EOL a long time ago. Otherwise, LGTM!
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"?
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).
Update: current version still works with the base install of fast404.
@graber feel free to add @Anybody and me as maintainers!
You can check our profile for references. Cheers!
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.
Setting to Major, as the hotfix will do the trick for now.
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!
Awesome! 🙂👍
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.
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.
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.
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!
Yes I did!