Account created on 3 January 2011, about 13 years ago
  • Internet Communication and Service Engineer at SicseΒ  …
#

Merge Requests

Recent comments

πŸ‡³πŸ‡±Netherlands Watergate

@clemorphy, thanks for testing. You can download a plain diff from the merge request. See the plain diff link under the issue fork list at the top of this page (left beside the green "MR !25 mergeable"). For additional information, see https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... β†’

πŸ‡³πŸ‡±Netherlands Watergate

When performing ./vendor/bin/phpunit --configuration web/core --filter 'testFindCallerFromDebugBacktrace$|testDiscoverService' web/core/tests/Drupal/Tests/Core, after applying the changes of the merge request:

PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Testing /var/www/app/web/core/tests/Drupal/Tests/Core
...                                                                 3 / 3 (100%)

Time: 00:00.154, Memory: 70.00 MB

OK (3 tests, 3 assertions)
πŸ‡³πŸ‡±Netherlands Watergate

I've updated the merge request so that this Micon Linkit sub-module is installable on Drupal 10. I've mimicked the Micon Link module's core_version_requirement, as this is a dependency.

Do I understand correctly that this issue is not postponed as ✨ Linkit for Link field Fixed is fixed?

πŸ‡³πŸ‡±Netherlands Watergate

The patch of #29 didn't apply to Drupal 10.2. So, I've updated the patch.

I've removed the proposed changes in the FormElement class, as they have already been added to Drupal (10.2).

The points made by #2863308-24: Content language negotiation not working for entity autocomplete β†’ must still be addressed.

πŸ‡³πŸ‡±Netherlands Watergate

I understand that having a composer.json is not required.

From Add a composer.json file β†’ :

It is better not to provide a drupal/core version requirement in composer.json because Drupal's composer facade will generate the appropriate metadata based on the info.yml file.

And although Drupal 8 (and Drupal 9) are EOL, I would suggest to apply the patch of #5. Create a release compatible with all versions and consider creating a 2.0.0 version of this module that only supports Drupal 10.

Marking RTBC for the patch provided in #5; let me know if further help is needed.

πŸ‡³πŸ‡±Netherlands Watergate

Watergate β†’ changed the visibility of the branch 2.x to hidden.

πŸ‡³πŸ‡±Netherlands Watergate

Watergate β†’ changed the visibility of the branch 1.0.x to hidden.

πŸ‡³πŸ‡±Netherlands Watergate

Sorry, I now understand that the Media diff UI plugin sub-module uses deprecated functionality in Drupal 10. I will, therefore, change the core_version_requirement for that specific sub-module. In case the Media diff UI plugin sub-module is needed in a Drupal 10 installation, the 2.0.0-alpha1 version of this module can be used, but the introduced access changes should be reviewed closely (as indicated in #3369369-11: Automated Drupal 10 compatibility fixes β†’ ).

πŸ‡³πŸ‡±Netherlands Watergate

Changes are merged.

πŸ‡³πŸ‡±Netherlands Watergate

I have updated the merge-request, by reverting the commit that changed the access checking. So that we can create a Drupal 10 compatible release using the 1.0.x branch, and continue the work for new functionality on the 2.x branch.

πŸ‡³πŸ‡±Netherlands Watergate

@Mingsong, you can add me as a maintainer. I will create the Drupal 10 compatible release and help where needed.

πŸ‡³πŸ‡±Netherlands Watergate

@Mingsong, thanks for your reply. I understand your point that "dev"-versions shouldn't be used in production. But the functionality of πŸ“Œ Remove deprecated MediaRevisionAccessCheck in Drupal 10 Fixed , which has been merged already, is blocking a Drupal 10-compatible release for this module.

I propose rolling back the functionality that needs explicit testing, using that to create a Drupal 9 and 10 compatible release, and tagging it as 1.1.0. In combination, make a 2.0.0-alpha1 version compatible with Drupal 10 (and 9?), which contains the functionality that needs explicit testing. We can then use the project page and release notes to point users for feedback on the changed functionality (that is included as an alpha release).

Please let me know if you agree with the above proposition and need help.

For others who want to install this module in Drupal 10, I've attached a small patch to this issue, which can be used with the mglaman/composer-drupal-lenient and orakili/composer-drupal-info-file-patch-helper Composer plugins to ease Drupal10 upgrade.

πŸ‡³πŸ‡±Netherlands Watergate

I looked into this issue to get this module working with Drupal 10. While updating/applying the patch provided by the Project Update Bot in the issue fork, I noticed that in πŸ“Œ Remove deprecated MediaRevisionAccessCheck in Drupal 10 Fixed , the core_version_requirement already had been updated, but this is not included in a release yet.

πŸ‡³πŸ‡±Netherlands Watergate

Watergate β†’ changed the visibility of the branch 3369369-automated-drupal-10 to hidden.

πŸ‡³πŸ‡±Netherlands Watergate

Watergate β†’ changed the visibility of the branch 3369369-automated-drupal-10-1 to active.

πŸ‡³πŸ‡±Netherlands Watergate

Watergate β†’ changed the visibility of the branch 3369369-automated-drupal-10-1 to hidden.

πŸ‡³πŸ‡±Netherlands Watergate

I can confirm the patch applies cleanly.

πŸ‡³πŸ‡±Netherlands Watergate

@orakili, thanks for the Composer plugin. It helped me/us a lot!

πŸ‡³πŸ‡±Netherlands Watergate

Does the #65 requires some manual action?

After applying the patch, everything should work as before. Using {{ content }} should still produce the same output as before. The extra functionality of the patch is that you can output specific blocks like {{ content.page_title }} and {{ content|without('page_title') }}.

Please let us know if you can reproduce the problem using a clean install.

πŸ‡³πŸ‡±Netherlands Watergate

Thanks a lot for the patch(es); this functionality was/is missing. I can confirm that everything works perfectly on Drupal 10.1.6.

I agree with the nitpick in #3178202-59: Render blocks later, so they can be placed individually in a region template β†’ and think that @group Render makes the most sense (and is in line with the rest of the test classes in the namespace). I've updated the patch with the small change :)

πŸ‡³πŸ‡±Netherlands Watergate

I've implemented the suggested change and opened a merge request.

As stated in the issue description, the problem seems similar to #2478099: Unable to read EXIF data when uploading to S3 Bucket. β†’ . I've opened a new ticket since that ticket is about the Drupal 7 version of the module, and there seems to have already been some work/updates in the Drupal 8 (9/10) port/version.

πŸ‡³πŸ‡±Netherlands Watergate

The patch provided in #3126330-3: Apply image style dimensions with HTML attributes β†’ didn't apply anymore to the latest version, so I've updated the code and created a merge request.

πŸ‡³πŸ‡±Netherlands Watergate

Watergate β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands Watergate

Stumbled upon the same problem when attaching a paragraph β†’ field to (fixed) block content.

I agree with @vidorado #3272253-5: Fixed blocks are not visible to guest users β†’ that this "solution" might be too permissive/friendly. The infinite loop is indeed a problem; the solution might be to implement the same strategy as used by the BlockContentAccessControlHandler::checkAccess():

AccessResult::allowedIf($entity->isPublished())
  ->orIf(AccessResult::allowedIfHasPermissions($account, [
    'access block library',
  ]))
  ->orIf(AccessResult::allowedIfHasPermissions($account, [
    'administer block content',
  ]))

Where the $entity->isPublished() should be the referenced/included block content object.

The provided patch didn't apply cleanly anymore (to the latest dev-version), so I've opened a merge-request (already).

πŸ‡³πŸ‡±Netherlands Watergate

Watergate β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands Watergate

Added file name suggestion for the provided code fragment, improved grammar and solved typo's.

πŸ‡³πŸ‡±Netherlands Watergate

Works like a charm on both D10.0 and D10.1 :)

πŸ‡³πŸ‡±Netherlands Watergate

I have added the missing information in the config/schema/heading.schema.yml file and opened a merge request.

πŸ‡³πŸ‡±Netherlands Watergate

I agree that the compatibility claim in the potx.info.yml is currently incorrect.

If the 8.x-1.x version of this module should still support Drupal 8, and therefore Drush 8, then I would propose creating a 2.x version/branch for this module that supports Drupal core ^9.3 || 10 (and thereby Drush 10 and 11). The 8.x-1.x should be made compatible with Drupal 8 again (this means that the changes made in πŸ“Œ Drupal 10 compatibility fixes for potx Fixed should be partially rolled back).

If Drupal 8 support can be dropped in this module's 8.x-1.x version, the proposed commit tag would be a friendly gesture. To prevent confusion, the potx.drush.inc should be removed since only Drush 10 or higher should be supported.

πŸ‡³πŸ‡±Netherlands Watergate

Good point; I think you are right, @Eric_A. However, similar changes were already made in #3285613 (i.e., rewriting the deprecated code in the potx.drush.inc file, but for extracting strings from module files).

Let's use #3354887 to discuss which core and Drush (combinations) are supported and the most suitable strategy.

πŸ‡³πŸ‡±Netherlands Watergate

I've updated the issue description based on the above comments.

Resolved merge conflicts on the original merge-request, so that it can be merged to the 3.x branch, and made some improvements (such as adhering to coding standards, ensuring the test proceeds, changing the name of the interface and adding a default implementation).

Added an extra merge-request for the 4.x branch, containing the same changes as for the 3.x branch.

πŸ‡³πŸ‡±Netherlands Watergate

I've updated the patch of #16 ✨ Drush Support for extracting strings in the theme files Needs work to be compatible with Drupal 10 (and the latest version of Drupal 9).

The change involves the deprecation of drupal_get_path() in Drupal 9, see https://www.drupal.org/node/2940438 β†’ . In case older versions of Drupal should still be supported, instead of $path = \Drupal::service('extension.list.theme')->getPath($theme);, something like $path = function_exists('drupal_get_path') ? drupal_get_path('theme', $theme) : \Drupal::service('extension.list.theme')->getPath($theme); (but in that case this change/fix should also be applied on the code handling extracting string translations from modules).

πŸ‡³πŸ‡±Netherlands Watergate

Since pregmatch has been replaced (i.e., reverted) to fnmatch (see commit https://git.drupalcode.org/project/potx/-/commit/bb7d5c0a15c2954b3c5e8a5...), this issue is not relevant anymore. Therefore, I'm marking this issue as "outdated".

πŸ‡³πŸ‡±Netherlands Watergate

I've added the module to my Drupal 10 project by running `composer require 'drupal/sendgrid_mailer:^1.1'. However, when trying to enable the module I got: "Unable to install modules: module 'sendgrid_mailer'. Its dependency module 'sendgrid_api' is incompatible with this version of Drupal core.".

The problem is that in `composer.json` the `"drupal/sendgrid_api": "^1.0",` (is required again), while only the 2.0 branch of the SendGrid API module is compatible with Drupal 10.

Was `"drupal/sendgrid_api": "^2.0",` rolled back in https://git.drupalcode.org/project/sendgrid_mailer/-/commit/e8e001e98743... to support Drupal 9 again? If that is the case, I propose to remove the Drupal 10 compatibility from the 1.1.x branch and create a new branch e.g. 1.2.x (or simply 2.x) that is compatible with Drupal 10 (similar to the SendGrid API project).

I've also applied the change as suggested by @larowlan in #10. Although I understand the original comment in the code, the fix should be applied/made in the `PhpMail` class itself.

The changes are pushed to https://git.drupalcode.org/issue/sendgrid_mailer-3289557/-/tree/3289557-... and a merge request can be made once a new branch (i.e., 1.2.x or 2.x) is created.

πŸ‡³πŸ‡±Netherlands Watergate

Watergate β†’ made their first commit to this issue’s fork.

πŸ‡³πŸ‡±Netherlands Watergate

I can confirm the patch works as intended.

Production build https://api.contrib.social 0.61.6-2-g546bc20