Account created on 22 January 2007, about 18 years ago
#

Merge Requests

More

Recent comments

🇧🇪Belgium swentel

nvm, was able to create a merge request myself

🇧🇪Belgium swentel

Ha, funny! Could you create a merge request from that, the fix is working indeed!

🇧🇪Belgium swentel

Display Suite relies on the core plugin since a very long time now, and no issues are reported on that integration. I'm fairly sure the others are fine as well, so this can be closed imo.

🇧🇪Belgium swentel

indielogin.com seems to work for me with patch 3

getting a notice though: Deprecated function: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Drupal\indieweb_indieauth\Entity\IndieAuthAuthorizationCode->getScopes, so will fix that while we're at it

🇧🇪Belgium swentel

Hmm the invalid state is on indielogin.com? Haven't tested that patch yet, but I'll double check somewhere next week!

🇧🇪Belgium swentel

New patch, suble change where the 'me' optional, but, in case it's there muse validate. Fixes the tests as well.

🇧🇪Belgium swentel

Thanks for the detailed report! I'm pretty sure I've tested this at some point in time where it probably worked, but indeed, now it doesn't. I did a quick test with https://micropublish.net/ too for instance, and nothing, so either the spec has been changed, or it's more relaxed, hard to say.

However, attached is a patch which made me able to authenticate on indielogin.com and micropublish.net. From a security point of view, it doesn't seem to be less secure not validating me, so I think it's fine.

Tests will probably fail, need to look at this (locally, they don't run anymore on the DA test infrastructure due to https://www.drupal.org/project/drupalci_environments/issues/3387737#comm... 📌 Split PHP image into php(cli/apache) and yarn(node/nightwatch) Needs review ), but it would be great if you could verify the login works now.

🇧🇪Belgium swentel

Thanks for all the maintenance love too! :)

I'm amazed the module usage is still so low, because it really is wonderful, so thank you for all the fantastic work!

🇧🇪Belgium swentel

Pushed! (sorry, needed patches file for composer.patches for a couple of projects quickly, so did that first :)

🇧🇪Belgium swentel

Also, those validations area introduced in 10.2.0, so it's fine no?

🇧🇪Belgium swentel

10.2.x is EOL, so just up the constraint of the module to 10.3 - that's good enough imo. A new branch seems way overkill for that.

🇧🇪Belgium swentel

Ah crap, that was stupid sorry :) New patch.

🇧🇪Belgium swentel

There's actually no guarantee there is a translation too for the entity. If not, it will crash

This is the current fix I have for now, but I'm not sure if it's the proper one. But at least it doesn't crash for now.

(which reminds me I should make a follow up for the formatter_settings crash, but close on a deadline atm)

diff --git a/src/Plugin/CustomFieldWidgetBase.php b/src/Plugin/CustomFieldWidgetBase.php
index 51e544cdf..00e5b26ea 100755
--- a/src/Plugin/CustomFieldWidgetBase.php
+++ b/src/Plugin/CustomFieldWidgetBase.php
@@ -112,7 +112,7 @@ public function widget(FieldItemListInterface $items, int $delta, array $element
       $langcode = $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
       $is_default_translation = TRUE;
       $is_translatable = $field_definition->isTranslatable() && $field->getWidgetSetting('translatable');
-      if (!$entity->isNew()) {
+      if (!$entity->isNew() && $entity->hasTranslation($langcode)) {
         $entity = $entity->getTranslation($langcode);
         $is_default_translation = $entity->isDefaultTranslation();
       }
🇧🇪Belgium swentel

New release will install. It means the testbot won't be able to run anymore, but that's ok for now. See 📌 Allow lcobucci/jwt 4.3.0 for PHP 8.3 support Active

🇧🇪Belgium swentel

Remove the sodium replace and compat, but that means the testbot will fail always now

🇧🇪Belgium swentel

Actually, I've released 8.x-1.25 :)

Let's meet in 📌 Allow lcobucci/jwt 4.3.0 for PHP 8.3 support Active for the sodium extension.

🇧🇪Belgium swentel

Laste update: it's actually another commit that is still in dev that updated composer.json, that one is not in a release yet. Sorry for the noise!

🇧🇪Belgium swentel

Note: dev release indeed has support for D11 (added in 📌 Automated Drupal 11 compatibility fixes for indieweb Needs review ), but not release yet, need to fix the sodium stuff :)

🇧🇪Belgium swentel

More or less duplicate of 📌 Allow lcobucci/jwt 4.3.0 for PHP 8.3 support Active - I thought the sodium polyfill could work, but I have to double check again.
I /think/ it should work locally, but the testbot has a problem installing it, so it's kind of a chicken/egg problem.

🇧🇪Belgium swentel

Sorry for the delay, last commit, or attached patch should do it! Test (which I can only run locally for some reason) has a subtle change which indeed failed, but with the changes in Expert.php then passes!

🇧🇪Belgium swentel

yes, running 3.1.3

Using the debugger, it happens on the formatter settings for a file, using the file_default formatter.

Looking at the code, it looks like this line is the problem: https://git.drupalcode.org/project/custom_field/-/blob/3.1.x/src/Plugin/...

It's totally possible that $settings[$name] does not contain the formatter_settings key, which happens in this case for a file.

🇧🇪Belgium swentel

Getting an error which is related:

TypeError: Drupal\custom_field\Plugin\Field\FieldFormatter\BaseFormatter::createOptionsForInstance(): Argument #3 ($formatter_settings) must be of type array, null given, called in /home/drupal/web/modules/contrib/custom_field/src/Plugin/Field/FieldFormatter/BaseFormatter.php on line 463 in Drupal\custom_field\Plugin\Field\FieldFormatter\BaseFormatter->createOptionsForInstance() (line 101 of modules/contrib/custom_field/src/Plugin/Field/FieldFormatter/BaseFormatter.php).
diff --git a//src/Plugin/Field/FieldFormatter/BaseFormatter.php b//src/Plugin/Field/FieldFormatter/BaseFormatter.php
index 4cd0936e5..0e351ac0f 100755
--- a//src/Plugin/Field/FieldFormatter/BaseFormatter.php
+++ b//src/Plugin/Field/FieldFormatter/BaseFormatter.php
@@ -460,7 +460,7 @@ protected function getFormattedValues(FieldItemInterface $item, string $langcode
         $format_type = $formatter_settings['format_type'];
       }

-      $options = $this->createOptionsForInstance($custom_item, $format_type, $formatter_settings['formatter_settings']);
+      $options = $this->createOptionsForInstance($custom_item, $format_type, $formatter_settings['formatter_settings'] ?? []);
       $plugin = $this->customFieldFormatterManager->getInstance($options);
       $value = $plugin->formatValue($item, $value);
       if ($value === '' || $value === NULL) {
🇧🇪Belgium swentel

Been bitten by this one too, this makes it work indeed!

🇧🇪Belgium swentel

Bitten by this one as well, the patch makes sense, and it works at the moment. Would be great to have some security eyes on this one!

🇧🇪Belgium swentel

Been bitten by this one too, patch seems to resolve the issue, so RTBC for now!

(I wonder if bcc might have the same problem?)

🇧🇪Belgium swentel

Hmm there's already the concept of a 'Site wide actor' in the module, but that still means an entity of course. The activity entity currently has an owner field, but having no owner (basically, anonymous) could work. Sounds like a good feature request which could live in its own dedicated issue. Question is, as always, time of course :)

🇧🇪Belgium swentel

Ok, confirmed that it doesn't work when the node that is being deleted had a path alias, makes sense!

🇧🇪Belgium swentel

If you deselect it, that it will probably work indeed (but with the patch it will be ok too).

Question is: did you upload a file for that note? If so, than there is a problem though with the code itself, I'll test it myself as well.

🇧🇪Belgium swentel

It's configured on the activitypub types screen at /admin/config/services/activitypub/activitypub-type.
If you are using the standard note, check which field is assigned to the attachment fields (there's image, video and audio) (e.g. at /admin/config/services/activitypub/activitypub-type/note)

🇧🇪Belgium swentel

Hmm, interesting. Attached patch makes sure $file is actually a valid object. However, it's interesting that there's no file loaded. Did you upload one? And if yes, is it a media field or a file/image field?

🇧🇪Belgium swentel

Oh crap, you are right! Looking at the test, it enables the field item wrapper .. so, the test works by accident.
Looking at the code, the replacement could actually happen multiple times, which is completely unnecessary!
Oh lord, where was my mind :)

🇧🇪Belgium swentel

The problem is with XSS::filterAdmin

Ok I get it now. XSS::filterAdmin has always been there, but the problem is that for some reason the code in Expert.php fails to replace the token with the actual value. The token should have been replaced already at this point (which why I was confused since the test is green, but maybe the test is completely wrong too haha)

Will have a fresh look this weekend, thanks for the pointers.

🇧🇪Belgium swentel

Actually, adminTags does allow the a tag, oh well, will test more and add more tests!

🇧🇪Belgium swentel

Well, the test doesn't use the a tag, but span. Crap! So yeah, that's the problem indeed, I'm testing it with the wrong tags.

🇧🇪Belgium swentel

Interesting, especially because there's a test which works .. :)

@devarch in your example, that configuration happens on a block type right? Just to be completely sure.

🇧🇪Belgium swentel

The replace of the tokens already happens in Expert.php, unless there's something else I completely missed as well :)

🇧🇪Belgium swentel

I'm running that patch (on an older version still, oh dear), but works fine! The exeption is catched and logged by search api and (in this case) the node is indexed later on by cron, nice!

🇧🇪Belgium swentel

Merged, will push a new release in a couple of minutes.

I can't reproduce the history page not found error so far, and I don't think it's a problem with this module afaics.

Thanks!

🇧🇪Belgium swentel

Don't get why this happens, but removing the unused statement doesn't hurt of course.

🇧🇪Belgium swentel

How does that even happen? I don't have that problem.

🇧🇪Belgium swentel

Aha, page not found, I'll double check, because you never know our access checks interfere when loading the comment in that route.

🇧🇪Belgium swentel

@pearls: what's the warning? is it a PHP error or notice? And it doesn't happen when this module is turned off?

🇧🇪Belgium swentel

Hey nidhi27,

Update script is fine for me!

🇧🇪Belgium swentel

Looks good!

One thing that I have to think about is the removal of the original permissions (e.g. 'view unpublished comments on own content'). This means that projects updating to the new code will have to reconfigure their permissions (and probably crash as well I think because Drupal doesn't like non existing permissions anymore iirc). So either, we also keep those, or we'll need an update script.

My personal preference at the moment is to keep the original permissions too, no ?

🇧🇪Belgium swentel

Cool, and yes, makes sense. Merged, thanks!

🇧🇪Belgium swentel

No clue then. 5.0.x is not even supported anymore in the project settings on d.o, so not idea upgrade status thinks that this is an ok version, probably a bug in there than :)

🇧🇪Belgium swentel

Hmm, can't install latest release on my local machine, or on server, let's see how to get around this

🇧🇪Belgium swentel

Patch attached which should fix it, are you able to test it ?

🇧🇪Belgium swentel

Hmm, that's weird, but I think I know why, let me double check later!

🇧🇪Belgium swentel

Alright, committed, new release will be up in a couple of minutes.

🇧🇪Belgium swentel

Ah crap, my bad! Attached is a patch which should solve it, are you able to test it?

🇧🇪Belgium swentel

Should be fine now with release 8.x-3.26

🇧🇪Belgium swentel

Alright, merged in, new release in a couple of minutes.

Production build 0.71.5 2024