Account created on 30 September 2014, over 10 years ago
  • Creative Director at Pivale 
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom d.fisher

Amazing!!

🇬🇧United Kingdom d.fisher

I think they both need work still so I'd go with just tagging what has already been merged.

🇬🇧United Kingdom d.fisher

Ah. I've just refreshed myself. Videoask is a premium service and as a result gets set up as a subdomain of your own domain. If you were drupal.org for example and you wanted to use videoask as the custom subdomain your videos would live at videoask.drupal.org/s0m3rand0m1d

That regex is currently matching and HTTPS url that:
1. Starts with https://
2. Has at least one character after the domain
3. Ends with a path segment composed of letters, digits, or hyphens

This could indeed match pretty much any URL and isn't fit for purpose. Perhaps this particular plugin would need a page in the UI where you can enter the custom domain which hosts your videoask videos so that this can then be used to match?

🇬🇧United Kingdom d.fisher

Yeah this merge request needs rebasing and that regex looking at. Not sure why I did it like that in the end? I'll take another look.

🇬🇧United Kingdom d.fisher

Agreed. I think tagging a new alpha with D11 support is desperately needed. The whole idea of an alpha release is something stable enough to test but not recommended for production sites. This will at least allow more people to test the module on Drupal 11 and report any issues.

🇬🇧United Kingdom d.fisher

Haha I must have accidentally changed it after all!!

@avpaderno I get a 403 when I visit: https://www.drupal.org/admin/people/advanced?name_op=%3D&name=dishabhadr...

🇬🇧United Kingdom d.fisher

Sorry search "drupal.org project ownership" in the project autocomplete box!!

🇬🇧United Kingdom d.fisher

No need for a new issue. Where the "Add new comment" section is you can edit the issue metadata where the project is currently Media Entity Link (3212769) but you can change that to be Project Ownership and it will move this issue from this module to the issue queue for project ownership and it will then be picked up by the team there to assign maintainers. The project field is an autocomplete so just start typing "Project Ownership" and it should show up. I would do it for you but when I've done that in the past the issue has been kicked straight back because the OP has to move the issue to the Project Ownership issue queue.

🇬🇧United Kingdom d.fisher

@danrod are you still interested in co-maintaining this module?

🇬🇧United Kingdom d.fisher

This issue is ready to be moved to the project ownership issue queue now. This should be done by the OP. Please use the Project autocomplete and change it from Media Entity Link to Drupal.org project ownership and avpaderno should be able to add you as a maintainer!

🇬🇧United Kingdom d.fisher

I've added a MR for this. Here's the patch.
https://git.drupalcode.org/project/config_pages/-/merge_requests/45.patch

After digging in to the code I can see that 'administer config_pages types' permission does add some security around clients accidentally clicking the button, but it might be nicer to have this broken out into its own permission. Let me know what you think either way!

🇬🇧United Kingdom d.fisher

Can this be merged and released? Tested locally on Drupal 11. It's a quick and simple change that would make this module Drupal 11 compatible?

🇬🇧United Kingdom d.fisher

In terms of moving this issue on it might be helpful to create a branch from this issue and have Drupal log which config file contains the offending NULL instance so that it's easier to pinpoint where the error is occurring?

🇬🇧United Kingdom d.fisher

Alternatively you could find offending item (in my case the taxonomy term view) and edit it in the UI and then save it to see if the system can resolve the config on its own and then export that item and reimport. This is what fixed it for me!

🇬🇧United Kingdom d.fisher

I resolved this by looking at: web/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php

After line 289: $data = $config_importer->getStorageComparer()->getSourceStorage()->read($name);

Put a var_dump: var_dump($data);

This allowed me to search for instances of NULL and I found one in views.view.taxonomy_term.yml where I had:

dependencies:
  -  config: null

Removing the - config: null line fixed my import issues.

THIS IS NOT A ONE FIX FITS ALL SITUATION!

You need to find exactly where in your config there is an instance of NULL that shouldn't be there and get rid of it!!

Good luck. Hope this helps!

🇬🇧United Kingdom d.fisher

I like this idea. The issue with relying on meta tags is that a lot of sites won't use them. I wonder if we could leverage Puppeteer to grab a screenshot of the webpage and save it as a file, but I believe this requires having node.js.

Here's the documentation for when I come back to this:
https://pptr.dev/guides/screenshots

This isn't going to be straightforward but it's a really good idea that I'd definitely like to look into.

🇬🇧United Kingdom d.fisher

This feature request has been captured in a duplicate ticket: https://www.drupal.org/project/media_entity_link/issues/3244504 Ideas for module: capture screenshot, store page info Active .

Closing as a duplicate.

🇬🇧United Kingdom d.fisher

As vladimiraus seems absent and danrod has opened another issue I will move this to closed (outdated).

🇬🇧United Kingdom d.fisher

2.0.4 released with support for PHP 8.1 and 8.2 reinstated.

🇬🇧United Kingdom d.fisher

Never mind. I think I was just using the wrong variable in the gitlab-ci template.

🇬🇧United Kingdom d.fisher

Just tried this and the runner was hanging. Looked here:
https://www.drupal.org/docs/getting-started/system-requirements/php-requ...

Drupal 11 only supports PHP >= 8.3.

Perhaps we should have two releases - one for Drupal 11+ and one for Drupal 9/10? Ideally we'd avoid having two points of maintenance for this module though as I'm currently the only active maintainer!

🇬🇧United Kingdom d.fisher

Whoops!! Sorry. I'll get on this!

🇬🇧United Kingdom d.fisher

I have referenced this issue here:
https://www.drupal.org/project/projectownership/issues/3514042#comment-1... 💬 Offering to co-maintain Media Entity Link Active

🇬🇧United Kingdom d.fisher

Cross referencing these two issues:
https://www.drupal.org/project/media_entity_link/issues/3504411 💬 Offering to co-maintain Media Entity Link Active
https://www.drupal.org/project/media_entity_link/issues/3520583 💬 Offering to co-maintain Media Entity Link as well Active

where danrod: https://www.drupal.org/u/danrod has also offered to be a co-maintainer

As I am not the project owner I am unable to add further maintainers to the project.

🇬🇧United Kingdom d.fisher

Hi. I know you're busy working on this. Is there any update on getting Drupal 11 support into a stable release?

🇬🇧United Kingdom d.fisher

Any update on a stable release for this? I opened this issue: https://www.drupal.org/project/country_path/issues/3508079 📌 Drupal 11 release RTBC which got closed with a message saying a release was planned for the end of March? Thanks.

🇬🇧United Kingdom d.fisher

Have managed to reproduce this. The field storage is left orphaned. However would that not be the desired behavior if the field has been used on another media type? I created a new media type called "another_link" and then uninstalled the module. My new media type "another_link" was removed when the module was uninstalled which I'd argue is perhaps not desirable.

My question is what do we expect to happen when the module is uninstalled?

  1. Any media type using the field.storage.media.field_media_entity_link should be deleted along with the field storage?
  2. A check should be performed to see if any other media types are using the field storage and if so do not delete the field storage or any media types that are using it except for the one provided by the media_entity_link module?

I'm going to set this as maintainer needs more info until we can reach a consensus on what the correct behavior should be!

🇬🇧United Kingdom d.fisher

I've tested this on a clean install and it looks good!

🇬🇧United Kingdom d.fisher

Fixed on the dev branch. Please feel free to test and report back and I'll tag a release!

🇬🇧United Kingdom d.fisher

Not getting anywhere with this one. Tests are not my strong point so setting to Needs Work! Anyone who has a deeper knowledge, feel free to duck in!

🇬🇧United Kingdom d.fisher

@danrod this is fine with me. I think the more maintainers the better so this module doesn't become stale again. I'm only assigned the developer role though which means I can't add maintainers I believe so you might need to create another issue like this one so that @avpaderno can assign you as a developer also, or perhaps we can avoid the waiting times and get you added now? Either way I'm of the opinion that more maintainers is better for the longevity of this module!

🇬🇧United Kingdom d.fisher

Pleased to say this is finally fixed and released!

🇬🇧United Kingdom d.fisher

Hopefully happening soon over here:
https://www.drupal.org/project/projectownership/issues/3514042 💬 Offering to co-maintain Media Entity Link Active

🇬🇧United Kingdom d.fisher

Leaving open (fixed) as per bot's recommendation.

🇬🇧United Kingdom d.fisher

darren.fisher changed the visibility of the branch 2842405-hide-empty-langcode-wrapper to hidden.

🇬🇧United Kingdom d.fisher

darren.fisher changed the visibility of the branch 2842405-empty-language-langcode to active.

🇬🇧United Kingdom d.fisher

darren.fisher changed the visibility of the branch 2842405-empty-language-langcode to hidden.

🇬🇧United Kingdom d.fisher

Thank you. And that takes us to the first full release 1.0.0!!

🇬🇧United Kingdom d.fisher

Tested and added another fix to 1.0.x for tp_select_data(). Let me know if all looks good to you and I'll close this and tag a new release.

🇬🇧United Kingdom d.fisher

Thank you @priscarabelli. This was sat on my to do list. I've added a test to cover this and updated the readme and implemented this in to the menu, select, and table functions so that full stops don't come out when using these twig functions. I've merged to 1.0.x if you want to test it works for you? Will tag a release once we've had a change to test the dev branch.

🇬🇧United Kingdom d.fisher

Thank you so much! I really appreciate your review.

🇬🇧United Kingdom d.fisher

Cross referencing this issue: https://www.drupal.org/project/projectapplications/issues/3485070 📌 [1.0.x] Twig casings Active where I have applied for the ability to opt projects into security advisory coverage which will likely be required in order to become a maintainer of this module.

🇬🇧United Kingdom d.fisher

Tagged and released. Thanks everyone.

🇬🇧United Kingdom d.fisher

Just popping back and marking this as needs review again.

I've recently created another module which contains more PHP than this one. See: https://www.drupal.org/project/twig_placeholders

I've been following all the standards and hopefully this will give you a better idea of my capabilities. I've been working with Drupal for over 10 years professionally for a variety of enterprise organisations and am now looking to be more involved in the contrib space and being able to opt projects in to security advisory coverage will be a major step. For example I have an application open to maintain / co-maintain an abandoned module: https://www.drupal.org/project/projectownership/issues/3514042 💬 Offering to co-maintain Media Entity Link Active and have recently become a co-maintainer on the nodeaccess project https://www.drupal.org/project/nodeaccess .

In addition to this I've also added additional commits to the twig_casings project and intend to continue updating and supporting these modules for many years to come.

🇬🇧United Kingdom d.fisher

Marking fixed. Will tag a release once the tests are passing on 1.0.x

🇬🇧United Kingdom d.fisher

Thanks for testing the MR @priscarabelli!!

🇬🇧United Kingdom d.fisher

This looks good. Going to merge to dev and then fix the pipeline there as it's a bit easier for my workflow. I like the way the arguments work so happy to merge in as is.

🇬🇧United Kingdom d.fisher

Added the lorem ipsum service to this. Something I've realised is that the lorem ipsum service assumes you want sentences and add full stops to the end of each generation so I will open a separate issue to deal with this and later implement it here.

🇬🇧United Kingdom d.fisher

This is looking really good. I've updated the README in this branch to document this new functionality and changed the default values in MenuDataExtension.php to match what is in the README.

Next steps:

  1. I'm going to look at using the lorem ipsum service to generate the menu titles.
  2. We'll want some tests similar to the other twig functions that cover the functionality provided by this new twig function.
🇬🇧United Kingdom d.fisher

darren.fisher made their first commit to this issue’s fork.

🇬🇧United Kingdom d.fisher

It seems this may be unrelated to the previous issue. I will roll this patch in to a merge request so it's easier to review and test in the gitlab pipelines.

🇬🇧United Kingdom d.fisher

Is this related to https://www.drupal.org/project/twig_tweak/issues/3427835 Add possibility to placeholder menu renders Active ?

🇬🇧United Kingdom d.fisher

Also the PHPStan errors are weird. They all state:

No error to ignore is reported on line XX

Here's the full error output:

------ -------------------------------------------- 
  Line   src/Command/SignatureFormatter.php          
 ------ -------------------------------------------- 
  47     No error to ignore is reported on line 47.  
 ------ -------------------------------------------- 
 ------ -------------------------------------------- 
  Line   src/UriExtractor.php                        
 ------ -------------------------------------------- 
  49     No error to ignore is reported on line 49.  
  83     No error to ignore is reported on line 83.  
 ------ -------------------------------------------- 
 ------ --------------------------------------------- 
  Line   src/UrlExtractor.php                         
 ------ --------------------------------------------- 
  72     No error to ignore is reported on line 72.   
  112    No error to ignore is reported on line 112.  
 ------ --------------------------------------------- 
 ------ --------------------------------------------- 
  Line   src/View/BlockViewBuilder.php                
 ------ --------------------------------------------- 
  148    No error to ignore is reported on line 148.  
 ------ --------------------------------------------- 
 [ERROR] Found 6 errors                                    

When I inspect the first one I see:

// @todo Fix this.
// @phpstan-ignore-next-line

I'm not sure why these are here? I'm sure there is some reason but it appears the PHPStan would not error on the following line anyway so maybe these can be removed? Again seems like this is probably a separate issue?

🇬🇧United Kingdom d.fisher

Just fixed the phpcs errors introduced by these changes. The PHPUnit failures I'm unsure of the correct approach.

Line 32 of tests/src/Kernel/Command/DebugLoadersTest.php contains the following assertion:
self::assertStringContainsString('/twig_tweak/templates/', $display);

The test is asserting that the output must contain the string:
"/twig_tweak/templates/"

But the actual output is:
"modules/custom/twig_tweak-3427835/templates/"

I'm not sure if this needs some sort of partial string check update using regex as the -3427835 relates specifically to this issue branch which means phpunit will always fail on any branch. This feels like a separate issue altogether.

What do you think?

🇬🇧United Kingdom d.fisher

darren.fisher made their first commit to this issue’s fork.

🇬🇧United Kingdom d.fisher

Thank you @guiu.rocafort.ferrer. Any update on the release?

🇬🇧United Kingdom d.fisher

The maintainer for this module has been unresponsive for some time and the module is currently in limbo. I would like to be considered as a maintainer so I can ensure this module stays up to date and has a regular release schedule! Thank you for your consideration.

🇬🇧United Kingdom d.fisher

Bumping this. Tomorrow this issue will be escalated to the Drupal.org project ownership issue queue.

🇬🇧United Kingdom d.fisher

Thanks for this. Merged and fixed! Will add tests for this new functionality on the 1.0.x branch so I can test thoroughly locally and then will tag a new release!

🇬🇧United Kingdom d.fisher

I think I've cracked it. Tests now pass. I must stress though I'm pretty new to the inner workings of this module so please can this be thoroughly reviewed and tested before being merged? The logic has had to be altered a fair bit from the patch for 1.x due to large changes in the module between branches!

🇬🇧United Kingdom d.fisher

New MR to resolve this issue in 2.x. Please test thoroughly!!

https://git.drupalcode.org/project/nodeaccess/-/merge_requests/18

There are PHPUnit errors. I will try and get to these shortly!

🇬🇧United Kingdom d.fisher

Also I believe all work is now taking place on the 2.0.x branch and as it is still an issue let's start there and if needed cherry pick the change to the 1.x branch although 1.x is no longer listed as a recommended version on Drupal.org.

🇬🇧United Kingdom d.fisher

I'm going to roll this in to a merge request so we can test it more thoroughly.

🇬🇧United Kingdom d.fisher

I'm not sure if this lies within the remit of the nodeaccess module? Can you provide some more information about your request and I can investigate if this issue belongs in another queue or does indeed fall under the remit of the nodeaccess module? Thank you.

🇬🇧United Kingdom d.fisher

Is this the same issue as:
https://www.drupal.org/project/nodeaccess/issues/3511078 🐛 Updating from 1.x to 2.0.1@alpha purges settings and grant field data Active

If so we should also mark one of these as closed (duplicate).

🇬🇧United Kingdom d.fisher

Is this the same issue as:
https://www.drupal.org/project/nodeaccess/issues/3509391 💬 Upgrade to version 2 deletes existing permissions Active

If so we should also mark one of these as closed (duplicate).

Production build 0.71.5 2024