Looks good!
Looks good and waiting for other Drupal CMS related changes.
Added the Piwik PRO logo.
I also thought about the version constraint but seems I didn't think far enough. The original thought I had would was to put something like there:
>=2.3.5 <3.0
That would force it to over 2.3.5 but not over 3.0.0 but then there might of course be situations like 2.4.0 etc. but if we still allow minor versions, then that would also work.
But this works for me also. Thanks for the extra effort!
Changed now and the pipelines pass with warnings. The warnings seem to be related to files outside of this issues scope.
I disabled the next major tests for now although they were only warnings so if you prefer, we can only pin the gitlab template to that ref and still keep OPT_IN_TEST_NEXT_MAJOR enabled.
I adjusted the gitlab-ci.yml and the composer.json and added the following variables:
OPT_IN_TEST_CURRENT: '0'
OPT_IN_TEST_PREVIOUS_MAJOR: '1'
Composer install and phpunit now pass with Drupal 10. It still tries to install Composer dependencies for Drupal 11 which seem to fail. I'll dig a bit more from the documentation if I can disable also the composer install for the next major version and only run composer install for the previous major.
The documentation was a bit confusing because according to https://www.drupal.org/drupalorg/blog/gitlab-ci-templates-will-make-drup... → the shift has not yet happened from Drupal 10 to Drupal 11 but 📌 Update templates so 11.0 is the default/current branch RTBC seems closed and fixed. At least the setting seems to now skip Drupal 11 phpunit correctly and and only test Drupal 10.
I'll mark this as Needs review for now.
Is there a related issue for the pipeline update I could mention here?
And thanks for the elaboration.
I created the MR now. I added the minimum version for 2.3.5 and removed the old dev option.
Hope I did it as intended.
Can you elaborate where was this raised as a security issue? In Drupal security issue tracker that is not public?
I agree with berdir that this might be out of scope for the module to totally handle because bumping the composer.json version might not be enough but you need to do work outside of Drupal to update simpleSAMLphp. But not all developers have for example Dependabot monitoring in place so I would think it would make sense for the module to safe guard a bit and only allow installing a safe version of the module? Especially since the CVE score seems to be quite high.
It's not a security issue in the actual module so I don't think this is worth a SA-CONTRIB notice or anything like that. This is why I also opened a public issue for this one. I have seen previously similar issues where a third party library has a vulnerability and there have been similar fixes that for example composer.json is locked to version v2.3.5 so that it only supports safe versions of the module.
What is the reason to keep supporting old EOL versions of simpleSAMLphp or dev-simplesamlphp-2.1 version of it?
This was reviewed and fixed at the same time.
This task can be now continued after 📌 Deprecate sync tags Active is merged and 1.1.1 released. It should make this task easier because all the sync tag code was removed.
The phpcs and unit tests are currently failing: https://git.drupalcode.org/issue/piwik_pro-3482343/-/pipelines/340566
Marking as Needs work.
Updated our composer.json packages with the following changes:
- Upgrading simplesamlphp/saml2 (v4.6.14 => v4.16.14)
- Upgrading simplesamlphp/simplesamlphp (v2.1.3 => v2.3.5)
- Upgrading simplesamlphp/simplesamlphp-assets-base (v2.1.19 => v2.3.2)
Everything seems to work fine as long as also the simplesamlphp is also the same latest version.
External links module has a good example where there is a notice for users already using this setting which is shown during install process: https://git.drupalcode.org/project/extlink/-/merge_requests/42/diffs#a2b...
We should implement a similar notice when we deprecate the current setting.
As the first step we could disable the setting from everyone and disable the setting with that notice. And after that we can remove the setting completely.
Just a small note but the comment from #6 is referencing this issue itself which might confuse users landing directly to this issue. :) I guess it should reference 🐛 Remove wrapping the last word in extlink span. Needs review .
Agreed that this approach seems very useful for card like components etc.
At least I am fine with this change since it's now well documented and clearly visible that this setting can be changed if there is an issue with the styling after upgrading. 1.x to 2.x is a major update so there can be breaking changes in my opinion as long as they are well documented which seems to be now done well.
Tested the patch and both enabled and disabled options.
+1 for RTBC with true as default but I'll let the people with the initial discussion make the final status change.
I will mark this task as postponed until we have solved 📌 Deprecate sync tags Active .
All conflicts and strings changed and tests are passing. Looks good. I also updated the module page to correct spelling.
Marking this as Needs work until the dependency injection part is changed.
Marking this as Needs work because of the missing translation.
Issue |#3354840] is closed and doesn't need work. We should progress with 📌 Deprecate sync tags Active first because that will remove a bunch of code from the module and that will make this task a lot easier to manage.
And yes, after #3486733 is done we need to update the tests to pass again.
Related to the ESLint warnings earlier. The script is coming from Piwik PRO directly. I think we should keep the changes to those files to minimum if Piwik PRO in the future makes changes to the code it will be easier to diff the changes. Because of this I would put the original JS code format back with the linter errors and put the files into .eslintignore file. I would keep them formatted as they were earlier but just replace the Piwik domain etc.
Excellent. Thank you. Closing this then.
This issue has not been updated in two years. The initial problem motivation does not tell in detail what is the added benefit of this feature and what are the uses cases.
Also we would need to check if Piwik PRO currently still supports this and if there are any security implications here. Could someone for example inject a malicious script to the site.
This ticket should be checked along with 📌 Move JS snippets to separate script files Active because they are doing a bit similar things.
We encountered this same problem in two different sites after updating the module.
I would be willing to accept that we did a mistake once and forgot to export configurations once after the database updates but these sites were updated by two different developers and both sites are reporting mismatch for taxonomy terms.
Mismatched entity and/or field definitions
The following changes were detected in the entity type and field definitions.
Taxonomy termThe Publish state field needs to be installed.
The Unpublish state field needs to be installed.
I was able to reproduce it once where the search was returning the item while the checkbox was on but that was months ago and no new reports have been submitted.
We have updated the module a couple of times and I have not received further bug reports from the client. We updated it again now with Drupal 10.3 so the information is so outdated that it cannot be reproduced anymore in our case reliably.
I think this could be closed and reopened if it still happens with the supported versions.
This change is so radical from the unit test perspective so we need to think about the unit tests a bit.
Basically the unit tests are now expecting a lot of the JS snippets are in the DOM and it's trying to find the string from the it and test that everything gets injected correctly.
You can find the current php unit tests from https://git.drupalcode.org/project/piwik_pro/-/blob/1.1.x/tests/src/Func... and I think we need to think about how they should be updated to support the external JS files.
The failing tests are
testPiwikProConfiguration
testSnippetVisibility
testSnippetVisibilityPagesInverted
testSnippetVisibilitySyncSnippet
testSnippetVisibilityChangedDataLayer
testSettingsForm
All the other errors seem to be related to the JS snippet not being visible in the DOM but there is one error that seems to indicate a larger problem:
7) Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testSettingsForm
Behat\Mink\Exception\ExpectationException: Current response status code is 500, but 200 expected.
It seems like the module settings page is now giving an error 500.
I will add the needs tests tag for now. These should be possible to also run the unit tests in local for easier debugging with https://github.com/ddev/ddev-drupal-contrib.
At least in our case the user role does have the View unpublished content for the problematic content type. They do not have the permission View any unpublished content. They also have the edit permission so they are able to open the node edit route. I was able to open the unpublished node in view mode but the editing was blocked by the error.
Adding the needed tests for this seems like the logical next step.
We actually experienced this issue also when upgrading the project from 1.x to 2.x. We were on a tight schedule and were not immediately able to fix the issue and actually did not notice this BC issue so we ended up downgrading the module back to 1.x. Usually we do contrib module updates when doing a large core update and this might mean a lot of contrib module updates at the same time so the time frame for testing each and every contrib module is pretty small because budget and time reasons.
These kinds of things are hard because although the breaking change is quite minimal (some styling broken) it's still a bit hard when it's visible in almost all the pages of the site.
I think it would make sense to keep the update path from 1.x to 2.x so that the old behavior does not change. I would vote for keeping the default value for the new option as false because as mentioned by @andyf this seems to add extra markup to existing sites doing the update.
I can also confirm that we have View unpublished contrib module installed in our project. But still notable that the setup was working fine in 10.2.x branch. Verified that we were running 10.2.7 before updating to 10.3.6. And after that update non-admins started to receive that error message when saving translations. Removing the patch from issue #2557469: Route validation does not preserve request method, defaults to GET → solved the issue.
I think this issue needs an issue summary update and verification if the problem still exists with supported Drupal versions.
Indeed. That problem should have fixed the issue so I am not 100% sure why it now surfaced for us with 10.3.6. Looking at issue [#2557469) and the patch #30 it seems to alter how the AccessAwareRouter works. Perhaps there was some change in in 10.3.x that caused the patch to work wrong.
Anyway I agree this issue needs summary and status update because it seems like 🐛 hook_node_grants implementations lead to a 'URL Alias' validation error when saving translated nodes. Fixed should have fixed the issue.
A small update related to our issue after updating to 10.3. The permission change for Link to any page did solve the problem but was probably not the original issue. While digging deeper I started to remove all core patches we had for 10.2 and finally arrived to removing the patch from issue #2557469: Route validation does not preserve request method, defaults to GET → and that seemed to fix the problem. It seems like that issue might be outdated and is already fixed in a separate issue.
I arrived here when debugging an issue that started after updating to Drupal 10.3. Our node translations started to work wrong and we were not able to save unpublished nodes. I originally thought the issue was with 🐛 Non-admins cannot save unpublished nodes with path alias Active and first tried solving the issue with Link to any page permission. But then I tried removing all core patches and removing the patch from #30 seemed to fix the problem.
This issue seems outdated because 🐛 AccessAwareRouter does not respect HTTP method Fixed is closed and no new reports have been filed. We originally added this patch to fix editing media items in CKEditor but that problem doesn't seem to exist anymore without this patch.
We are encountering this issue after updating to Drupal core 10.3.6.
We have disabled that certain roles can add content to menus and edit the path alias. This has been working before but after the update they cannot save nodes. It seems like the alias is not updating soon enough and it gives an error.
From the attached screenshots it's visible that the alias is correct but it's giving an error the original node ID alias.
If I give the Link to any page permission to the affected role it starts to work.
A couple of ideas for the road map that I received as feedback from a client.
1. At the moment when you start going through the found issues on the page, you can see the total number of issues found on the page but once you start going through the list you cannot anymore track your progress. Would be really nice to see how you progress the page for example just showing a pagination that at the moment you are on issue 5/10 found from the page.
2. At the moment it seems like you are able to navigate to the next issue but there is no way to navigate to the previous issue. Would be nice if there were both previous and next links. You can for example by accident click on the next button twice and then there is no easy way get back to the previous issue.
We tracked this down to our custom code where it was firing also in admin routes and causing an error. Closing this as not related to contrib code.
heikkiy → created an issue.
Merged to 2.2.x and ready for next release.
Merged to both 2.1.x and 2.2.x. Credited the fix to you.
Merging after approvals.
Would it be possible to get this rerolled for 10.3.x also as mentioned in #94?
Checking the patch against 10.3.x seems like there are large changes at least in ContentTranslationHandler.php.
The whole reroll check gives the following:
git apply --check 2971390-81.patch
error: patch failed: core/modules/content_translation/src/ContentTranslationHandler.php:534
error: core/modules/content_translation/src/ContentTranslationHandler.php: patch does not apply
error: patch failed: core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php:327
error: core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php: patch does not apply
I think this change could be done for both the new 2.2.x and previous 2.1.x branches. I will make a MR for both.
Then we could release 2.1.5 and 2.2.1 with the small backward compatibility fix.
Merged. Going to the next 1.1.0 release.
Looks good!
I can also report the patches seemed to fix the initial language issue after deployment but like mentioned in #23 the English labels made a return a bit later. We also discussed this further in Drupal Slack and there I mentioned that the returning of the English labels seemed to have something to do with cron. When I ran the cron tasks manually on the server it seemed to have an effect on the labels. So basically running drush cr command first to fix the labels and then a cron task again seemed to revert the labels back to English.
Unfortunately I haven't been able to track a specific set of commands to reproduce the issue. I guess this issue is so hard to fix because the problem can happen in several ways and steps of the workflow.
Excellent, thanks.
A couple of things came to my mind related to testing
1. What happens if the system roles change? New roles might not cause issues but what happens if a role is removed?
2. Do we need to take caches into account? Does the current implementation take those into account if a user with a role to see the snippet first visits the page and then right after a second user without the same role visits the page. Currently the code does not seem to implement any cache contexts when loading the snippet. Not sure if this can be handled with unit tests so any ideas are welcome.
heikkiy → created an issue. See original summary → .
Merged.
I wonder if it would be a separate issue that the view filter plugin seemed to affect also a view that was not using the Simple hierarchical select selection type but the normal Dropdown? I am not that familiar how the view filters in the core work in the backend but it seems weird that the code execution goes this far to get a SQL error when the selection type is not used.
This change looks good to me now looking at https://git.drupalcode.org/issue/piwik_pro-3482195/-/blob/3482195-replac....
Added some improvement ideas to the MR. I think it could include additional information and get ideas for example from https://git.drupalcode.org/project/search_api_solr/-/blob/4.x/README.md and https://git.drupalcode.org/project/metatag/-/blob/8.x-1.x/README.md?ref_....
But these are out of scope for this issue and we can create a new issue for improving the project readme.
Marking as Needs work still because of the failing tests and readme changes.
Thanks for the MR. This looks very good to us and was about the same functionality I was also thinking.
The ECA idea same mostly so that it would be possible to add a more powerful rules engine to the module but agreed that it is a bit overkill.
This does look like a new feature so I think it would justify a 1.1.0 release. For that it would make sense to create a 1.1.x branch and make that as the default branch and target branch for this MR.
I will try to handle that as soon as possible.
But in the mean while the module also includes tests that should be passing: https://git.drupalcode.org/project/piwik_pro/-/blob/1.0.x/tests/src/Func.... Currently the pipeline is failing: https://git.drupalcode.org/issue/piwik_pro-3480154/-/pipelines/315963/te....
The tests should be updated and we should also add information about role visibility to the project readme: https://git.drupalcode.org/project/piwik_pro/-/blob/1.0.x/README.txt. A bit out of scope but that file should be also renamed as readme.md.
We encountered this same issue in a view that was not actually using the view filter but was still encountering the error with a negative depth.
The patch worked so marking this as RTBC.
This is a very good idea. We need to think about a little how to implement this. It could be a simple setting in the module page that would allow you to select which roles can see the embedded scripts but that would be mostly custom code.
There are also other contrib solutions like https://www.drupal.org/project/eca → that allows conditions but it seems a bit heavy sided for this functionality.
Other ideas and ways to implement this are welcomed.
Hi @hartsak.
Thank you for the message. Yes, the old implementation for the settings was a bit confusing and we also mixed them a bit when we were trying to figure out how to support both v1 and v2. Sorry for the bad UX that originally caused this issue.
I think in the process we initially thought that people by default have been using v1 and we didn't want to suddenly switch the default to v2 without them acknowleding any other change they might need to do for example in the Cookie information template.
The change should be still be visible in the config that you see the correct v1 or v2 version so the change should be pretty easy to catch.
I am trying to think if you should in the future enforce v1 or v2 if both have been selected before.
We encounted this same issue with Storybooks and tested that the same change fixes the issue. So +1 for RTBC.
Could this be merged and a new release done both to drupal.org and https://www.npmjs.com/package/@drupal/once?
Fixed in 1.0.4 release.
Hi @yevko, nice to see you.
No, both sites have the default language as English.
Our system.site.yml has
_core:
default_config_hash: yXadRE77Va-G6dxhd2kPYapAvbnSvTF6hO4oXiOEynI
langcode: en
uuid: 01602f5c-032b-4c67-a1da-b7bba1207a45
name: 'Example site'
mail: 'noreply@example.site'
slogan: ''
page:
403: ''
404: ''
front: /node/1
admin_compact_mode: false
weight_select_max: 100
default_langcode: en
mail_notification: noreply@example.siteBut indeed the language negotiation has been set so that the site by default opens the Finnish site and not the English one in language.negotiation.yml:
_core:
default_config_hash: uEePITI9tV6WqzmsTb7MfPCi5yPWXSxAN1xeLcYFQbM
session:
parameter: language
url:
source: path_prefix
prefixes:
en: en
fi: fi
domains:
en: ''
fi: ''
selected_langcode: fiMy feeling is that because the site's default language is set to English sometimes Drush takes that as the default language and changes Finnish labels into English. This does not happen the other way around that English site shows Finnish labels. The problem still gets fixed by clearing all caches in the admin UI.
Unfortunately my comment from #51 was not the final verdict. We have still seen this issue after deployments. Mostly the problem still happens with the multisite where term names and field labels are in wrong language after deployment. Everytime a manual cache clear from the UI fixes the issue. It seemed like the patch from 🐛 Configuration being imported by the ConfigImporter sometimes has stale original data Fixed might have even made the problem more visible because with that patch we were also seeing wrong labels in the default site when usually they are limited to the multisite instance.
I'll report more findings if we are able to track down the issue further. Of course any debug or test instructions that we can run and report here would help.
Excellent. Thanks a lot for the extremely quick response!
Great. I can ask one of my colleagues to test the patch and mark it RTBC if everything looks good.
We have a couple of projects where we have been planning to take this feature in to use but I don't know the timetable yet. Most likely during the autumn. It would make it easier to plan for the feature if there was an official release available but we can live without it for now.