Account created on 27 September 2007, over 17 years ago
  • Technical Account Manager at Acquia 
#

Merge Requests

Recent comments

🇳🇿New Zealand Josh Waihi

Could you expand on what would require rearchitecture? DAM already has upload APIs. The Drupal module just needs to be configured with an upload profile, and then it could upload assets to the DAM.

Once uploaded, the media browser would need to be refreshed to see the images now available.

In the Drupal CMS, users are accustom to adding media via the Drupal UI. I'd strongly advocate to continue this convention, even if it means initiating the process from Drupal but carrying out steps through the DAM UI.

🇳🇿New Zealand Josh Waihi

I can't save Text Format configuration forms in the admin UI in Drupal 10.3 with this module enabled. Changing the schema from config_object to mapping (as this MR did) seemed to fix the problem.

I don't think I understand why this MR wasn't merged?

🇳🇿New Zealand Josh Waihi

@jurgenhaas, can this also be merged into 2.0.x?

🇳🇿New Zealand Josh Waihi

For that to work, you don't need to manually edit any yaml file, that should all be done by that hook.

I needed to revert my config so I could test a state where the hook would actually change the config.

Pipeline fixes done.

🇳🇿New Zealand Josh Waihi

Update hook added. Ready for another review.

🇳🇿New Zealand Josh Waihi

I tried testing drush eca:update:

  1. I updated my model config in YAML and removed the "group" configuration setting.
  2. Then used drush config:import to persist that configuration into the database
  3. Then I ran drush eca:update expecting it to update configuration in the database
  4. Then I ran drush config:export expecting it to recreate the "group" configuration setting with an empty value

However, config:export did not introduce any changes indicating eca:update did not work as expected.

🇳🇿New Zealand Josh Waihi

TIL when the target branch is changed in an issue, it also changes the diff/patch (which I was using in my composer patches for a project). So attaching a static patch for 1.1.x for reference (accepting it won't be added).

🇳🇿New Zealand Josh Waihi

Re-closing as I was able to take the feedback about using views and apply it. I got a little confused because the External Entities module doesn't allow you to build views with those entity types. However, for my use case, I need a view of nodes referencing a given external entity and I was able to do that and leverage the view in ECA.

🇳🇿New Zealand Josh Waihi

I don't know how to approach the update hook here. Is this something you can add or give me some guidance on how to write it?

🇳🇿New Zealand Josh Waihi

I am re-opening because the use case (external entities) does not work with views. Entity query is the only way to make this work.

🇳🇿New Zealand Josh Waihi

Sure I'm happy to have you added as a maintainer. Perhaps we can connect at somepoint to discuss a 2.x branch. I'd like to add the ability to conditionally apply cache control rules (e.g. For matching URL patterns). Perhaps Rules module support.

🇳🇿New Zealand Josh Waihi

Hi DieterHolvoet, can you give me an idea of what interests you have as a maintainer?
I see you have a lot of contributions on your profile - do you have time to be a maintainer? Admittedly, my time is limited too.

Something I am cautious about with this module is to limit UI configuration to help minimize potential issues users might get themselves into using this module. Though more advanced developers (who might have a better sense of what they're doing) can override UI config using config exports.

🇳🇿New Zealand Josh Waihi

I did some testing of the above patch in #3404804 and it worked as expected but I did find one bug.
In the UI, when you select an authenticate user to have a permission, all other roles inherit that permission. If that permission was not explicitly set for a scope role, then the role doesn't provide the permission even though the UI suggests implicitly selects it.

As a work around, I had to uncheck the permission for the authenticated user, check the permission for the role, then recheck the permission for the authenticated user.

🇳🇿New Zealand Josh Waihi

After a bit more debugging I found it did work. I just need to set additional permissions on the role scope I'd set.
Thanks for the help. Can close this out in place of #3397590

🇳🇿New Zealand Josh Waihi

I applied the patch from https://www.drupal.org/project/simple_oauth/issues/3397590#comment-15296074 📌 Add method to check if scope has permission Needs review but that didn't seem to resolve the issue.

🇳🇿New Zealand Josh Waihi

How about an additional checkbox for that conversion (in a separate issue)?

The ideal experience would be one where you could enable expert mode (e.g. toggle slider?) in the UI and without reloading the page, the form converts drop downs into integer inputs. That would likely require some form AJAX work so Drupal could rebuild the form with new input type for the form elements.

In lieu of that, a checkbox for export mode and a submission of the form to rebuild the form with integer inputs would work too.

I think we can do that in this issue and you can amend your current MR :)

🇳🇿New Zealand Josh Waihi

Hi @Shifali Baghel, I don't quite understand why we need to move the settings to under system.performance. It seems cleaner to keep the module's configuration under its own namespace to prevent collisions with other modules and mitigate potential core bugs using another module's namespace.

🇳🇿New Zealand Josh Waihi

Thanks for the contributions @BramDriesen,

Hmm, why not add a checkbox under a accordeon box "Expert settings" that enables those options?

I am making an assumption here that if you're expert enough to fine tune cache control, you're also exporting config to code (e.g. with drush config:export). And as such, you could manipulate the settings in code there and bypass the form UI. But that does render the UI completely unhelpful so I am open to your suggestion to make the UI support an expert mode.

That mode should probably transform the field inputs from drop down options to integer fields where the expert can arbitrary set the TTL in seconds. Though presents might be still helpful for the expert, having presents only in the form of dropdowns would be too restrictive.

🇳🇿New Zealand Josh Waihi

+1 - can we get this comitted?

Production build 0.71.5 2024