andras_szilagyi → made their first commit to this issue’s fork.
Generally speaking looks good, just needs a fix and a few minor improvements.
I'm adding a patch with the current state in case anybody is relying on the MR as a patch
I've opened a child issue to get the pipeline for this issue going on the new CI, also set up the CI so that in tests in various contexts so we can catch any issues and breaking changes, seems to pass with both facets 2 and 3, so I went with that and merged.
merged
I've :
- added ddev for easy development
- set up testing in different variants to be covered
- fixed eslint, phpcs, and (some) phpstan complaints
- Ported the cod from parent ticket as well as left the 2.x for BC
- Added a dedicated test for the facets 2.x
andras_szilagyi → created an issue.
andras_szilagyi → made their first commit to this issue’s fork.
claudiu.cristea → credited andras_szilagyi → .
ready for review
Personally I think we should go for solution C) because the word/concept "source" implies that its the original, in Drupal a translated menu item is not the original, so for me it makes sense to not create "sources" from translated menu items. Sources should only come from the sites default language
andras_szilagyi → created an issue.
andras_szilagyi → created an issue.
andras_szilagyi → made their first commit to this issue’s fork.
ready for review
I've added more test coverage for us to see the way mimeMailUrl behaves with the scenarios mentioned in #19, after I refactored the image detection using a centralized method that uses more core API
andras_szilagyi → made their first commit to this issue’s fork.
Fixing credits
- remove the tracking data rebuild from track_usage_config_update
- when entering the config edit form show a warning banner that says any change to the config might require full tracking data rebuild
- after submitting the form detect if there where configuration changes, if yes show a warning saying that you need to rebuild the tracking data with a link to the bulk update in the settings page
andras_szilagyi → made their first commit to this issue’s fork.
andras_szilagyi → made their first commit to this issue’s fork.
andras_szilagyi → created an issue.
andras_szilagyi → made their first commit to this issue’s fork.
Looks good
And as I suspected, this resolved the issue, see https://git.drupalcode.org/issue/contact_storage-3535501/-/pipelines/545193
andras_szilagyi → created an issue.
andras_szilagyi → created an issue.
andras_szilagyi → created an issue.
andras_szilagyi → created an issue.
andras_szilagyi → created an issue.
I confirm the issue is present in 4.x and the code in the MR fixes it.
Works for me, could confirm the issue in both scenarios, the one in the description and the one in #5.
@dxvargas, @smustgrave requested test coverage, in case you can oblige, please add that in a separate branch and in the current the same test so we can see the test fail and resolved by the fix.
andras_szilagyi → created an issue.
claudiu.cristea → credited andras_szilagyi → .
ready for review
I've created a test branch that proves the issues, as you can see it fails,
and a fix branch that makes the tests pass.
andras_szilagyi → created an issue.
andras_szilagyi → made their first commit to this issue’s fork.
andras_szilagyi → created an issue.
All affected are fixed, ok for me
The js file is not covered by FunctionalJavascripy, that worried me the most, but after manually testing the changes it works as expected, the pipeline changes are good.
The fix worked for me aswell
andras_szilagyi → made their first commit to this issue’s fork.
works
The test fails for D11 but that is out of scope, the pipeline run and the yml looks good
Thank you for your work, it is merged
Looks good
Thank you all, I've merged
I've reverted adding the patch as it does not seem to help, and instead added caching the account per user. Lets see the result of this.
Even if I set ->setCacheMaxAge(0); the behaviour persists, it seems like Drupal is not checking the observers access before its rendering the page, or before its caching the entity or view mode
I've pushed some commits to check if the issue is solved by this patch, but it seems to bring up new issues, I'll look more into it
andras_szilagyi → made their first commit to this issue’s fork.
I confirm the issue reported, I reviewed the related issue and it looks good, so I will add a line in this modules readme about it so users know to use it.
The code here also looks good.
andras_szilagyi → made their first commit to this issue’s fork.
Indeed if I check "custom permissions" without this fix then there is nothing happening. With the fix a table of permissions pop up.
I confirm the issue and fix.
Looks good
Looks good
Works well, looks good
andras_szilagyi → created an issue.
Looks good
Yes, looks good
Looks good, works fine also in my tests
Looks good
It's ok @damienmckenna, we are moving forward.
andras_szilagyi → made their first commit to this issue’s fork.