Toronto 🇨🇦
Account created on 2 May 2006, almost 19 years ago
#

Merge Requests

Recent comments

🇨🇦Canada colan Toronto 🇨🇦

Here's a Composer-friendly patch for 6.2.x, which assumes that #3452986-18: Update jquery.intl-tel-input to v23.0.10 and add option for i18n has already been applied.

🇨🇦Canada colan Toronto 🇨🇦

Here's a Composer-friendly patch for 6.2.x.

🇨🇦Canada colan Toronto 🇨🇦

Accidentally changed some things; just setting them back.

🇨🇦Canada colan Toronto 🇨🇦

I thought this was a support request, and someone provided an answer in #5. So it seems resolved to me.

(I don't think this is the same ID problem we had in v3, 🐛 Newly created entity "cannot have a URI as it does not have an ID" Active . If it is, someone from v2 can reopen.)

🇨🇦Canada colan Toronto 🇨🇦

Resolved merge conflict. Problem was that we're hacking on the file modules/webform_toggles/js/webform_toggles.element.js that's no longer there due to 📌 Deprecate the use of drgullin/icheck Active . I simply removed the file. If folks are interested in making this change in Webform Deprecated , they can dig it out of the commit.

🇨🇦Canada colan Toronto 🇨🇦

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

🇨🇦Canada colan Toronto 🇨🇦

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

🇨🇦Canada colan Toronto 🇨🇦

Added the Views issue to the next beta, but not sure if we should cut another release before that. There's been lots of activity lately.

🇨🇦Canada colan Toronto 🇨🇦

@guignonv: What do you think? Tests are still passing, so that's good.

🇨🇦Canada colan Toronto 🇨🇦

If someone makes me a co-maintainer, I can help with this stuff.

🇨🇦Canada colan Toronto 🇨🇦

This is really a documentation issue as it's not obvious how to do this. Here's what we need to do:

🇨🇦Canada colan Toronto 🇨🇦

colan created an issue.

🇨🇦Canada colan Toronto 🇨🇦

I think we can merge this after tests are passing.

🇨🇦Canada colan Toronto 🇨🇦

@guignonv: So the merge request (MR) is good so far? No major changes required?

@sugen: I would uncomment it and keep it it in. If it does nothing for now, that's fine, but at least we won't have to remember to add it back in later if/when we actually do need it.

🇨🇦Canada colan Toronto 🇨🇦

Amazing work; thanks!

I took a look at the code and it makes sense. We'll try it here, and get back to you. (I just turned the branch into an MR for easier viewing.)

🇨🇦Canada colan Toronto 🇨🇦

This one's back on we've confirmed that it happens even after we're returning the IDs and getting 200 OK.

It's with a Single Storage Client: POSTing/creating is successful, but then there's a WSOD on the front end. Coming at it from another way, GET/fetching data fails (and results in the same WSOD).

If we comment out these lines, it works fine:

So it's coming from ExternalEntityForm->save(), the two toLink() calls.

🇨🇦Canada colan Toronto 🇨🇦

Well, it's better than getting an error, right?

🇨🇦Canada colan Toronto 🇨🇦

This same error message is discussed in a core issue ( 🐛 Drupal\file\IconMimeTypes doesn't handle NULL mimetypes Active ), but that seems to more about actually missing mime types. This one here appears to be caused by the way EE is handling it.

🇨🇦Canada colan Toronto 🇨🇦

Or the DB setting is wrong.

🇨🇦Canada colan Toronto 🇨🇦

I'm mentoring Andrew, and I worked on this with him.

🇨🇦Canada colan Toronto 🇨🇦

Marking this as a duplicate of 🌱 Save between steps Active . This one's older, but is on an old branch.

🇨🇦Canada colan Toronto 🇨🇦

From #3178227: Save between each step , which I marked as a duplicate:

We used a hook_form_alter to add this:

    $form['actions']['next']['#validate'][] = '::validateForm';
    $form['actions']['next']['#validate'][] = '::submitForm';
    $form['actions']['next']['#validate'][] = '::save';
    $form['actions']['back_button']['#validate'][] = '::validateForm';
    $form['actions']['back_button']['#validate'][] = '::submitForm';
    $form['actions']['back_button']['#validate'][] = '::save';

(I agree with 🐛 Validation handler of the next button breaks other form validation Fixed , it feels strange to have this in #validate and not in #submit.)

But it would be nice to have a supported option directly in the module.

🇨🇦Canada colan Toronto 🇨🇦

Looks like if you merge it from here, it squashes it. I don't like that. Will do it from GitLab next time.

🇨🇦Canada colan Toronto 🇨🇦

I did the following:

  • Placed the token service in StorageClientBase so all clients can use it.
  • Fixed the namespace path to the token service.
  • Removed the Token module as dependency as this is Core functionality.

@guignonv: What do you think? Can we merge?

Would also be nice to cut a new beta after we get this in; I can do this if you'd like.

🇨🇦Canada colan Toronto 🇨🇦

Just a one-liner so far, but we should probably use dependency injection, injecting the `token` service into the RestClient.

🇨🇦Canada colan Toronto 🇨🇦

Needs to be a single patch, not several, or it won't apply.

🇨🇦Canada colan Toronto 🇨🇦

Here's a Composer-friendly patch.

🇨🇦Canada colan Toronto 🇨🇦

@filsterjisah & others: Besides fixing merge conflicts and getting tests to pass, I also refactored RefreshAccessTokenSubscriber so that the code is cleaner. Kindly review & try it? If all if well, we can RTBC this so it gets in soon.

🇨🇦Canada colan Toronto 🇨🇦

In that case, the RTBC was really for the non-v3 MR, not your patch, which needs review.

However, I just reviewed your code and it looks good. Also, we tested it as part of testing the other issue so RTBC actually does make sense now so leaving status as-is.

🇨🇦Canada colan Toronto 🇨🇦

Thanks for the update!

Okay, so we need to figure out how to incorporate both approaches without collision.

🇨🇦Canada colan Toronto 🇨🇦

Looks like this is now incorporated into 💬 Store and Use Refresh Token on Expiry Active so assuming that gets in, let's give credit to the folks here too. For now, I think we should direct our efforts over there.

🇨🇦Canada colan Toronto 🇨🇦

Pipeline is failing due to failed tests. Might just be that we need to add the new stuff to the schema:

1) Drupal\Tests\openid_connect\Functional\UserPermissionTest::testDisconnectFormSubmissionsForOthers with data set "administer openid connect clients permission" ('administer openid connect clients', false)
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for openid_connect.client.test_oidc_label with the following errors: openid_connect.client.test_oidc_label:settings.auto_refresh_expired_tokens missing schema
/builds/issue/openid_connect-3327440/web/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
/builds/issue/openid_connect-3327440/vendor/symfony/event-dispatcher/EventDispatcher.php:206
/builds/issue/openid_connect-3327440/vendor/symfony/event-dispatcher/EventDispatcher.php:56
/builds/issue/openid_connect-3327440/web/core/lib/Drupal/Core/Config/Config.php:230
/builds/issue/openid_connect-3327440/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:260
/builds/issue/openid_connect-3327440/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:486
/builds/issue/openid_connect-3327440/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:239
/builds/issue/openid_connect-3327440/web/core/lib/Drupal/Core/Entity/EntityBase.php:354
/builds/issue/openid_connect-3327440/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:617
/builds/issue/openid_connect-3327440/tests/src/Functional/UserPermissionTest.php:97
/builds/issue/openid_connect-3327440/tests/src/Functional/UserPermissionTest.php:114
🇨🇦Canada colan Toronto 🇨🇦

@rp7: Would you kindly let us know what's left to do here? Thanks.

🇨🇦Canada colan Toronto 🇨🇦

I think most of that token functionality is in core now, except for the token browser (for that you need the Token module ), but what I have in my head could be out-of-date. Actually this seems true as of Drupal 7! See https://www.drupal.org/documentation/modules/token .

So no, we don't need to add that dependency.

🇨🇦Canada colan Toronto 🇨🇦

That field is working fine, except that it needs to support replaceable Drupal tokens (as I mentioned above). I've opened a separate v3 ticket for this: Authentication Custom Header should support Drupal token replacements Active

🇨🇦Canada colan Toronto 🇨🇦

We started working on a patch for this, but ran into some other priorities. Will have to come back to this later if nobody else does.

🇨🇦Canada colan Toronto 🇨🇦

v2 is no longer recommended; see 📌 [META] Roadmap for v3.0 Active .

If nobody cares, we can mark it unsupported? I left it supported because there are recent commits to the branch.

🇨🇦Canada colan Toronto 🇨🇦

Marked v2 as supported again as I noticed some recent commits by other folks.

🇨🇦Canada colan Toronto 🇨🇦

Here was our conversation via e-mail exchange:

On Sat, Feb 8, 2025 at 10:35 PM I wrote:

Can we set v3 to Recommended and stop recommending v2? Or do you want to keep v2 as Recommended?

On 2025-02-10 06:02, Attiks wrote:

v3 is fine by me.

On Mon, Feb 10, 2025 at 4:10 PM I wrote:

Great. Are you still maintaining v2, or should we mark it unsupported?

On 2025-02-10 10:23, Attiks wrote:

Feel free to mark it unsupported

So I was able to:

  • Mark v2 as unsupported and marked v3 as Recommended.

I could not do these, because I don't have enough permissions (@guignonv: Can you do these as you're a Maintainer?):

  • Change the default branch to v3 because I'm only a Developer.
  • Deprecate the v2 Views module; I'm not a maintainer there at all.
  • Enabled automated testing (don't see the tab).
🇨🇦Canada colan Toronto 🇨🇦

The remote payload that was coming back was missing IDs so there was nothing to map with the Drupal entity IDs. So this error actually makes sense. Sorry for the noise!

🇨🇦Canada colan Toronto 🇨🇦

Thanks! What are your thoughts on v2?

🇨🇦Canada colan Toronto 🇨🇦

I used his contact form yesterday to ask about co-maintaining (and also pointed him to this thread), but haven't heard back yet.

🇨🇦Canada colan Toronto 🇨🇦

Updated status of Views support.

🇨🇦Canada colan Toronto 🇨🇦
+++ b/src/Plugin/ExternalEntities/StorageClient/Rest.php
@@ -180,41 +170,57 @@ class Rest extends ExternalEntityStorageClientBase implements PluginFormInterfac
+      $sequence[$exploded[0]] = $value;

Would it make sense to add a Token::replacePlain() in here to expand tokens in values?

In our case, users will each have an ID that they pass in for authentication & authorization so it'll be something like [User:remoteID].

🇨🇦Canada colan Toronto 🇨🇦

Thanks for the update! You've done an impressive amount of work here, and thanks for updating the Views doc page.

For my current project, we're starting to put together a proof of concept (PoC) for relying heavily on this module. So assuming that goes well, we should be able to help with stabilizing the issues that we're interested in, which includes:

For now though, I think we should do a few things quickly; I can do these if you make me a maintainer:

  • Deprecate the v2 Views module (which is no longer needed) in favour of the main module. (I'd also need maintainer access for this one.)
  • Switch the release recommendation on the main project page from v2 to v3.
  • Enabled automated testing in the project as I mentioned at 📌 Add basic test coverage Active .
🇨🇦Canada colan Toronto 🇨🇦

There's more happening in what if entity is not found Needs review . Maybe mark this as a duplicate of that one?

🇨🇦Canada colan Toronto 🇨🇦

Is this still relevant?

🇨🇦Canada colan Toronto 🇨🇦

For now, let's at least enable automated testing in the project settings so that we can see if patches apply. (There are a lot of v2 patches/MRs that may or may not work in v3.)

🇨🇦Canada colan Toronto 🇨🇦

This sounds overly complicated. What about simply having a config tab for the entity that allows you do choose which fields are entity references? By default, it could translate the remote ID value into the Drupal entity ID, and use that to make the reference?

For more complicated values in the reference field, it could allow for hook implementations that provide a translation in code.

🇨🇦Canada colan Toronto 🇨🇦

@burnellw_cit: Kindly provide an interdiff? If you can't, what changed? What's different about your patch?

🇨🇦Canada colan Toronto 🇨🇦

I didn't run a `diff`, but at a quick glance, the patch and the MR look the same. Let's work in the MR though? Patches can just be for Composer.

🇨🇦Canada colan Toronto 🇨🇦

Is this roadmap still relevant, or do we only need to talk about v3 now?

🇨🇦Canada colan Toronto 🇨🇦

Hi! I'm glad to see this is still active after we talked about it 10 years ago 📌 Make Views use SqlEntityStorageInterface Needs work . Would you be able to provide the status of Views integration?

🇨🇦Canada colan Toronto 🇨🇦

My client is telling me that a CC didn't help; needed the patch.

Back to needing a reroll...

🇨🇦Canada colan Toronto 🇨🇦

Rewrote for clarity.

🇨🇦Canada colan Toronto 🇨🇦

Added link to Views module.

🇨🇦Canada colan Toronto 🇨🇦

Clarified which one was the original.

🇨🇦Canada colan Toronto 🇨🇦

Removed duplicate issue links.

🇨🇦Canada colan Toronto 🇨🇦

Looks like this lesser-used client module could be moved into the more-used OpenID Connect / OAuth client module as a plug-in. Is there any reason not to do that?

See the comparison table .

🇨🇦Canada colan Toronto 🇨🇦

Added link to server module.

🇨🇦Canada colan Toronto 🇨🇦

Removed server module from the list.

🇨🇦Canada colan Toronto 🇨🇦

Sorry, my mistake. Turns out this is a server module, not a client one.

Production build 0.71.5 2024