🇨🇦Canada @Liam Morland

Regarding #10, if variant is null or otherwise empty, the variant attribute will not be set in gcds-link.twig.

If variant is to be default, it should just be left-out (or set to null). What is setting it to the empty string?

We will first need this on 6.3.x.

I did not correct "sigup" because this string may be used by others to modify the form. This should be fixed in the next major release.

liam morland → created an issue.

There are still several phpstan messages:

\Drupal calls should be avoided in classes, use dependency injection instead

Thanks. There are some previous tags left behind by this in the main repo.

This wasn't merged.

Thanks!

It would be better to find the root cause of this. The variant property should be set to null instead of empty string to not specify a value.

Please put your patch into an issue fork and merge request. Please add tests.

As a start, these changes get tests passing again.

liam morland → made their first commit to this issue’s fork.

This should be ported to 6.2.x. The commit can be cherry-picked, but there are more calls to fputcsv(). These were fixed on 6.3.x in commit 3c56216 for 📌 Fix broken tests Active .

Thanks!

Due to module support, it will be a while before we can move to Drupal 11.

Option 1:

$service = \Drupal::hasService('service') ? \Drupal::service('service') : NULL;

Option 2:

try {
  $service = \Drupal::service('service');
}
catch (ServiceNotFoundException) {
  $service = NULL;
}

Option 1 is more compact.

I would rather just do:

$service = \Drupal::service('service');

Or:

$service = \Drupal::service('service', ContainerInterface::NULL_ON_INVALID_REFERENCE);

(The code later, in a loop, tests if $service has a value and uses the service if it does.)

The merge request needs to be updated to target 11.x.

I am encountering this issue in hook_preprocess_HOOK(), so dependency injection is not possible. I'm using this to adjust the behaviour of the code based on whether or not another module is installed.

Is catching the exception better than just using \Drupal::hasService()?

I like the idea of adding a second param.

liam morland → created an issue.

@astonvictor what is the difference between the old merge request and the one you just opened? Which one does your RTBC apply to?

liam morland → made their first commit to this issue’s fork.

Created a merge request with the patch in #3.

liam morland → made their first commit to this issue’s fork.

This dependency should be updated to allow book 2.0.

"drupal/book": "^1.0 || ^2.0"

Please put your patch into an issue fork and merge request.

I have created a merge request with the patch in #4.

liam morland → made their first commit to this issue’s fork.

It is not clear to me that there are too many dependencies. Feel free to re-open with a specific proposal.

This is the contents of the branch 2948793-pdftk-security which branched-off from 7.x-1.14.

Yes, but please test that it works correctly.

Is this an improvement on ✨ WYSIWYG support on each table cell for better content experience Needs review ? If so, it ought to be in that issue.

Please use the null-coalescing operator, ??. Please correct the indentation.

liam morland → created an issue.

Some repo clean-up that could be done:

• Change the default branch in GitLab to 4.x
• Delete from the main repo obsolete branches for specific issues, such as 3513890-assertionerror-constant-contact
• Delete from the main repo previous tags

liam morland → created an issue.

liam morland → created an issue.

This seems to have fixed it for me.

@#37: The current issue is pretty straight-forward: Return int instead of string. Deciding what to do about the nulls is a bigger topic. So, I think it makes sense to surface that issue and put it into its own issue.

I have created 🐛 ::getCreatedTime() sometimes returns null instead of integer Active .

liam morland → created an issue.

Is there an existing issue about those?

Fix also required commit for #3022485: Undefined entity label impedes identifying FillPdfForm → .

liam morland → created an issue.

I installed the development version and the issue is fixed. Thanks!

@#10 I was referring to the patch in #8, which is not a patch to this module.

Would you consider a backport to 2.0.x?

(By the way, I noticed that tag 3.0.0-alpha1 is not on a branch. You could clean that up by merging it into 3.0.x.)

It would be helpful for this to be added as a patch that can be applied with Composer.

liam morland → created an issue.

liam morland → created an issue.

liam morland → created an issue.

This is the patch in the merge request re-rolled to apply to 4.0.0.

I have created a merge request with the patch in #4.

I think #6 intended to move this to RTBC.

I have created a merge request with the existing code in the issue fork.

liam morland → made their first commit to this issue’s fork.

Drupal 10 compatibility has been done in the 3.0.x branch in #3243437: Add CodeMirror support for CKEditor 5 → and 📌 Automated Drupal 10 compatibility fixes Fixed .

Drupal 11 compatibility has been done in the 3.1.x branch.

The fix in #9 fixes it for me. It has the same effect as #6 except that later updates, when the issue is fixed, will be installed.

Coding standards are not passing on the merge request.

Others, please test this.

Current state of merge request.

All checks are passing.

liam morland → made their first commit to this issue’s fork.

liam morland → created an issue.

liam morland → created an issue.

This appears to be handled by embedded_content_sdc module.

I suggest updating the "Historical support" page so that it matches the versions shown as supported on the project page.

Can you use git bisect to figure out which commit caused this issue?

I have created a merge request with the patch in #2. It seems to work on 4.0.x.

liam morland → made their first commit to this issue’s fork.