Account created on 18 June 2019, over 5 years ago
#

Merge Requests

More

Recent comments

🇩🇪Germany tgauges

I forgot to put this issue back into "needs review", please take a look at my response in the MR.

🇩🇪Germany tgauges

tgauges changed the visibility of the branch 3485655-3046447-ajax-focus-duplicate-forms to hidden.

🇩🇪Germany tgauges

The implemented test case is based on the existing test case which is disabled via 📌 [random test failure] Re-enable AjaxTest::testAjaxFocus() Needs work . So I'm not sure whether that new test case will be flaky as well...

🇩🇪Germany tgauges

I implemented a fix and a test case. The test case is not run because of 📌 [random test failure] Re-enable AjaxTest::testAjaxFocus() Needs work . I'm not sure how to continue because the flaky tests in #3396536 📌 [random test failure] Re-enable AjaxTest::testAjaxFocus() Needs work seem to only be happening in the pipeline and I don't have enough expertise for that.

🇩🇪Germany tgauges

I implemented a failing test. But that test is inside a test function which is currently being skipped because of 📌 [random test failure] Re-enable AjaxTest::testAjaxFocus() Needs work .

Should this issue be dependent on the other one?

🇩🇪Germany tgauges

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

🇩🇪Germany tgauges

I implemented an alternative solution which makes use of the new refocus-blur AJAX setting to refocus the last active element.

🇩🇪Germany tgauges

I fixed the relevant test but now another unrelated (?) test is failing. Could you take a look at it?

🇩🇪Germany tgauges

I created this issue to

  • have a discussion about the problem
  • have a (temporary?) solution available
  • have a discussion about the solution

Since my proposed solution can be seen as a breaking change, I left the merge request as a draft. Could there be other solutions?

🇩🇪Germany tgauges

I tested the changes against version 2.37 of the module and it works as expected. One could argue that this is not really a bug and merging this change would amount to a breaking change. In this case this issue lends itself to version 3 of this module, but that is for the maintainer to decide.

🇩🇪Germany tgauges

I created an issue fork with the existing patch applied. The tests seem fine. I'm currently in the process of testing the code in a project.

🇩🇪Germany tgauges

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

🇩🇪Germany tgauges

Here is a patch against 4.0.0 for reproducible builds.

🇩🇪Germany tgauges

I created an alternative solution, adding only and exclude lists to the purge method to give even more control. Here is a static patch for reproducible builds.

🇩🇪Germany tgauges

I created a new branch against 11.x which first sorts the translations by their source string and then the context. The context is needed for equal source string with different context.

Before review, the test should be extended to also test the case for equal source strings but different context.

🇩🇪Germany tgauges

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

🇩🇪Germany tgauges

This feature might be classified as a breaking change because the fixture loading behavior is changed. One could circumvent this by defining a new method/interface which can be voluntarily implemented in fixtures.

🇩🇪Germany tgauges

I created a patch of my current changes, also committed to the issue fork. The patch is only for replicable builds.

🇩🇪Germany tgauges

@joinso With 1.17 and Drupal 10.2 it should be possible to have references to paragraph entities. I'm not a maintainer of this module so maybe a new support request issue should be opened?

🇩🇪Germany tgauges

I had the same problem on one of my sites and applying the patch fixed the problem.
Changing the status to "Needs Review" for one or more additional reviews.

🇩🇪Germany tgauges

I think I might need some help with the update hook. I have some experience with updating contrib fields (added via GUI), but not base fields. Do you know some guide of existing code I can take a look at?

What also should be a part of the update hook in my opinion:

  • updating the view (if it exists)
  • removing broken values in the database
🇩🇪Germany tgauges

By beginning my implementation I noticed the form_data field also uses the `serialize` method but not the `map` field type. Form data probably doesn't include class instances but following the recommendation of the PHP documentation seems like a good idea.

🇩🇪Germany tgauges

I'm done for now with my contribution. There is some half-baked validation of the duration string because I noticed it will be better with https://www.drupal.org/node/3373502 which will only be available with Drupal 10.2 and I didn't want to raise the core version requirement. I did my best to document this in the code.

There are no tests.

🇩🇪Germany tgauges

@svendecabooter Would you like to have tests for this feature?

🇩🇪Germany tgauges

This might not have been a feature of the D7 version but it's still a nice feature. The patch applied to the current version in my project and works correctly.

🇩🇪Germany tgauges

I applied this issue's patch to version 2.0.x of the project. Only one location needed a small adjustment.

🇩🇪Germany tgauges

I implemented the necessary logic in an issue fork. Before writing tests, I'd like to know if this feature even has a chance to be merged.

🇩🇪Germany tgauges

I changed the existing code in 2921987-nesting-filter-groups to allow for nested condition groups.
With this change, one would still need to implement the views query alter hook, but at least now the nested conditions defined there will work.

I'm not sure about the consequences of this new implementation, I just implemented what seemed to make sense. Surely there will need to be new tests and maybe also documentation?

The next step would be to implement the definition of nested condition groups via the Views UI.

🇩🇪Germany tgauges

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

🇩🇪Germany tgauges

I simply removed the hook removing the option to configure a simple entity reference to a paragraph: https://git.drupalcode.org/project/paragraphs/-/merge_requests/82.

I kept the MR as draft because it probably goes against the goals of this module? If the option exists, there should probably be a warning or something if it is selected. What do the maintainers think?

🇩🇪Germany tgauges

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

🇩🇪Germany tgauges

Seems like it, yes. I added a link to eLogger in the description.
Thanks for showing me the module.
Can this issue be closed?

🇩🇪Germany tgauges

My changes in the issue fork work for my use case. I'm not sure that I found every use of the link dependency, please review.

🇩🇪Germany tgauges

The error seems to be caused by linkediting.js#L394 because the 'link' command ist not available as well. The same issue will pop up a few lines above.

🇩🇪Germany tgauges

I found out that adding core/ckeditor5.link to the entity_embed/entity_embed library and link.LinkEditing and link.LinkUI to plugins entry of the CKEditor 5 plugin PHP class fixes the initial problem.
This causes another error very unhelpful error:

TypeError: e is undefined
🇩🇪Germany tgauges

I implemented this feature in the issue fork. Please review :)

🇩🇪Germany tgauges

That's very strange. I'm not getting those errors with the same versions. I'll be closing this issue as not replicable.

🇩🇪Germany tgauges

Could you run phpcs --version as well, please? For me it's 3.7.2.

🇩🇪Germany tgauges

You seem to have run phpcs against the packaged module. Take a look at the source code and you will see that the errors do not exist: https://git.drupalcode.org/project/hide_non_editable_content/-/blob/1.x/hide_non_editable_content.info.yml

🇩🇪Germany tgauges

Are you using the docker compose service and the composer packages provided in the module source code?
If not, could you run php -v and composer show drupal/core (for the phpcs rules) in your environment?
For me its PHP 8.1.18 and versions : * 10.1.1 respectively.

🇩🇪Germany tgauges

Ah sorry, I mixed up phpcs and php-cs-fixer. I'm also using the latest version of squizlabs/php_codesniffer. Could it be a PHP version mismatch? This module is written for PHP 8.1 or above.

🇩🇪Germany tgauges

Thanks for the issue. Sadly I can't replicate the phpcs errors. Which version are you using? This module uses ^3.14 because of compatibility with other development dependencies.
Also you are removing readonly modifiers in your patch. Is that part of phpcs?

🇩🇪Germany tgauges

@Rajan Kumar Is it not enough to follow the recommendation in https://www.drupal.org/node/1068944#exclude and exclude these files when packaging a release?

🇩🇪Germany tgauges

@shashank5563 I created a .gitattributes file to ignore all development files when packaging a release. I'd like to keep the files inside the git repository for easier development.

🇩🇪Germany tgauges

Changing the target branch of the merge request

  1. broke our build
  2. made the MR have 700+ commits and 1000+ changes

I created a new merge requests against 9.5.x: https://git.drupalcode.org/project/drupal/-/merge_requests/3687.

Production build 0.71.5 2024