I forgot to put this issue back into "needs review", please take a look at my response in the MR.
tgauges → changed the visibility of the branch 3485655-3046447-ajax-focus-duplicate-forms to hidden.
Please take a look :)
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...
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.
The test is fixed.
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?
I implemented an alternative solution which makes use of the new refocus-blur AJAX setting → to refocus the last active element.
I fixed the relevant test but now another unrelated (?) test is failing. Could you take a look at it?
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?
@chiragp185, Do you have the option to test the code in https://git.drupalcode.org/project/entity_reference_delete_check/-/merge_requests/2?
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.
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.
Here is a patch against 4.0.0 for reproducible builds.
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.
I created a new branch and MR against 8.x-1.6: https://git.drupalcode.org/project/entity_embed/-/merge_requests/42
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.
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.
I created a patch of my current changes, also committed to the issue fork. The patch is only for replicable builds.
@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?
In my opinion this issue is solved by #3414495 📌 Change paragraphs_form_field_storage_config_edit_form_alter() to only discourage paragraph target references, not disallow Fixed . At least for Drupal versions 10.2 and higher.
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.
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
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.
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.
@svendecabooter Would you like to have tests for this feature?
I'd like to take a crack at this.
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.
I opened a merge request with my changes.
tgauges → created an issue.
I applied this issue's patch to version 2.0.x of the project. Only one location needed a small adjustment.
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.
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.
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?
Seems like it, yes. I added a link to eLogger in the description.
Thanks for showing me the module.
Can this issue be closed?
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.
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.
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
I implemented this feature in the issue fork. Please review :)
That's very strange. I'm not getting those errors with the same versions. I'll be closing this issue as not replicable.
Could you run phpcs --version
as well, please? For me it's 3.7.2
.
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
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.
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.
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?
@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?
@vishal.kadam I fixed the issues reported by phpcs.
@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.
Changing the target branch of the merge request
- broke our build
- 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.
tgauges → made their first commit to this issue’s fork.