Agreed. Looks much more modern and fits better with the blue color. Shame there is no thumbs up symbol in d.org issue comments but nice change and all in all looks like a good direction to go.
Is there something missing from marking this issue as RTBC? To me it seems like done based on the first comment proposed resolution and for example comparing https://www.drupal.org/project/webform → releases.
Reading through 📌 Remove “recommended” status for releases? Active and to me it seems like a good reasoning for the change. I guess we should then just get rid of the option to even mark projects as Recommended by the maintainer to avoid confusion? Because at the moment the Ui to mark a branch as the recommended might lead to confusion. The initial question from Slack came when we were wondering what happened in the project we are maintaining when both the supported and recommended versions were identical and very hard to distinguish which one to install. But I guess the aim is that there would be only one supported stable version and one additional development version for early adapters which makes sense to me.
I also agree with @greggles that colors are very personal and cultural choices and it's hard to pick colors that everyone is fine with. I guess normally it's common to write something like "Not recommended for production use." when some release is still in beta and alpha version but knowing that there are many cases when you actually need to run a Drupal module in production even in alpha state, that might also cause unneeded confusion.
I agree with previous comment and as also discussed in Slack. I also feel like there is some information that is communicated with color but not with a text alternative. I think we should verify that we have the same information available for screen readers that is communicated visually.
Marking as fixed and merged to 2.2.x.
I tested this patch and I encountered an error where the current date format D, d M Y H:i:s O gets translated based on the Drupal site language.
When previewing the date format in fi/admin/config/regional/date-time/formats/manage/rfc822_rss it gets translated as Ma, 03 Maa 2025 12:17:14 +0200 and this seems to break the feed and we get a validation error
"Failed to read requested Feed information. Details: Error in line 15 position 52. An error was encountered when parsing a DateTime value in the XML. clientRequestId: f40f103e-0cb9-4cc4-9d4d-827e9031b7c1"
I can verify the same issue. In our case the problem seems to be only with the channel pubDate. Actual item pubDates are correct because we have set a different date format in the field settings.
At the moment the MR pipeline seems to be failing also and I cannot seem to apply the patch either against 2.3 or 2.x-dev branch.
Gathering patches for root package.
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 1 update, 0 removals
- Upgrading drupal/views_rss (2.3.0 => dev-2.x 7f1e77a)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals
- Syncing drupal/views_rss (dev-2.x 7f1e77a) into cache
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
- Installing drupal/views_rss (dev-2.x 7f1e77a): Cloning 7f1e77a4f2 from cache
- Applying patches for drupal/views_rss
assets/patches/3492994--views-rss--rss-date-format.diff (#3492994 Change date format for RSS feed)
Could not apply patch! Skipping. The error was: Cannot apply patch assets/patches/3492994--views-rss--rss-date-format.diff
Indeed. Was already supposed to do that in the previous comment.
Merged to 1.2.x now and I resolved all threads from GitLab.
Marking as RTBC. Thanks!
Answering myself but seems like
https://www.drupal.org/project/views_rss →
does have the support for both description and content:encoded.
I quickly tested it with the core rss.xml view and it seems like it wraps both the description and content:encoded inside CDATA.
I think that might be a good option if you need more robust RSS support.
Would it make sense to create a follow up ticket where this would be possible to be configured? Or perhaps a contrib module. I guess it's quite regular that people are using RSS feed to share content and now that Drupal CMS has already launched I think the need might be growing to be able to build RSS feeds easily for different use cases. Would be nice if you could also configure the <content:encoded> as an optional value if you know you are exposing the RSS feed to a known reader that can support it.
My original interpretation also was that CDATA is more supported and it was news to me that encoded is fine also. Live and learn.
And regarding @longwave WP example, it's interesting that iframe for example is not passing validation.
Thank you. That clears it a lot.
@alexpott Not necessarily. I would like to perhaps just understand why the CDATA was deemed as a good solution in #3433 and then reverted back to the original version? I can understand the regression problem but was just wondering when a very old issue gets resolved and then it gets reverted. My original approach has been also that if you want to put HTML in your RSS feed that it should be inside CDATA but seems like encoding is fine.
I guess the change record still needs updating https://www.drupal.org/node/3504403 → because it says:
The CDATA event now only wraps the HTML output in CDATA tags.
One thing I noticed is that when I was testing our current feed in 10.3 that when validating the feed I got a warning about drupal-media not being a valid tag. This might be because of our current custom preprocess and definitely out of scope for this issue but just something that caught my eye when testing.
I would like to understand this change record a bit better. I was testing this with the core rss.xml.
We basically have previously implemented a preprocess where we add CDATA to certain RSS views and also alter the headers to use text/xml to pass RSS validation with CDATA.
After upgrading to 10.3 we started to see double encoding in the feed. I was testing this with the core rss.xml and I was seeing also the double encoding there. After investigation I noticed that core is now checking for the header to be application/rss+xml before it adds the CDATA wrapper to the description. So our problem was setting a wrong header type in custom functionality. Upgrading to 10.4 and removing our header alter seemed to fix the problem and it started to render HTML and CDATA correctly. But after I apply the patch from here the CDATA gets removed and it gives double encoding again.
So my question basically is that is it still possible to get CDATA in RSS views from ✨ Use CDATA in XML RSS Feeds (was: Malformed XML Feeds) Closed: outdated or is this functionality going to be completely reverted from core? To me this issue title seems to say that certain HTML tags are being stripped away but when looking at the MR, it seems that the CDATA functionality is going to be removed and the recommended alternative is to use for example https://www.drupal.org/project/rss_trust_cdata → ?
Just wanted to add a small detail that we have been using a view where we generate the RSS feed with fields instead of the display mode. We preprocess the feed and add CDATA to the description that is basically combining two fields together as the feed description.
After updating to 10.3 we started to see double encoding in the RSS item description.
I was able to also fix the problem based on comment #117 and using Markup::create() to define the CDATA content. I guess the RSS system now sanitizes the input also later and using Markup::create() marks it as safe to render.
We haven't yet updated to 10.4 so will be monitoring this issue for possible changes in the related regressions.
I think can be closed and reopened if other changes appear.
Thank you. I am still hesitant to approve this because all the unit tests were passing here https://git.drupalcode.org/issue/piwik_pro-3482342/-/jobs/4235376 even though the isVisible() check was not there. So something was wrong with the unit tests that they didn't catch the problem.
Excellent progress, thank you.
This looks really good. I think the pipeline linter and test validation updates are the only thing before we can move this to Needs review.
I tested the functionality in local and I am not able to reproduce the error with the changes from this MR anymore.
The current test pipelines https://git.drupalcode.org/issue/drupal-3416819/-/pipelines/405923 do seem to have errors still:
2) Drupal\Tests\options\FunctionalJavascript\OptionsFieldUIAllowedValuesTest::testOptionsAllowedValues with data set "List string with 0 value: Enter button" ('list_string', ['0', '1', '2'], true, 'Enter button')
Failed asserting that two arrays are identical.Pipelines are now working thanks to @mcgovernm. Good for review again.
I wasn't able to get it working in local but I did test it with https://github.com/justafish/ddev-drupal-core-dev.
Running ddev nightwatch tests/Drupal/Nightwatch/Tests/a11yTestPager.js seems to yield similar errors as with the pipeline Nightwatch tests.
It might be that my local test environment is not yet setup correctly for the full Nighwatch test so perhaps it would make sense for someone who has originally been able to run the tests to give this a go. It is still notable that the tests showed green before I made the most recent small out of scope change which IMO should not affect the Nightwatch tests.
No, not yet. That was on my to do list to try to reproduce the problem in local outside of the pipelines. Are there any instructions how to test that in the sandbox site?
Currently the Nightwatch test seems to fail after the latest commit. Seems a bit odd because it passed after the previous commit and the latest commit shouldn't change anything related to the test code.
Tried rerunning the Nightwatch part several times but it keeps failing.
Changed according to comments in the MR from @smustgrave. Changing back to Needs review.
pameeela → credited heikkiy → .
Rebased and all unit tests are green now. Marking as Needs review.
The current implementation would need a review although the failing pipelines are a bit of mystery because they don't seem related to code changes in this issue. Perhaps the issue branch needs a rebase?
I did notice that the comment from #9 is not taken into account in the current implementation where it still uses aria-label and not aria-hidden. Instead of using the aria-label that the screen readers might ignore, we could perhaps also use the visually-hidden class with a span to add more context for screen readers or CSS content property.
I pushed https://git.drupalcode.org/issue/piwik_pro-3482342/-/tree/1.2.x there now.
You can continue from that. Most likely it makes sense to start fresh and move the changes from the 1.x feature branch there because 1.2.x has a lot of new features.
I changed the target version to 1.2.x-dev. No need to assign the ticket to yourself @anish.ir. It's enough that you comment that you are working on it.
Fixed and merged.
Excellent thanks. I added a couple more automated tests to make sure that the default empty settings also work. We will try to get 1.2.1 released ASAP.
Added other fallback values also and all pipelines are now passing.
Thank you for reporting this. We noticed similar issues before releasing 1.2.0 but seems we missed this part.
I created a merge request and added fallback values for this and a couple more new configs.
Does this MR fix your issue?
I tested this with 2.0.x-dev. You are right, the functionality is totally different because the modal doesn't anymore have the Select button..
There is no need to fix this for an old alpha release. I am closing this as outdated.
phenaproxima → credited heikkiy → .
I think the problem might be because looking at https://git.drupalcode.org/project/autoban/-/tree/7.x-1.x?ref_type=heads the default branch is 7.x-1.x and the logo.png is not present there. The logo can be found from https://git.drupalcode.org/project/autoban/-/tree/8.x-1.x?ref_type=heads.
I was able to get it showing in another project by switching the default branch in Gitlab to be the one with the logo.png. This was mentioned in 📌 Update logo for compatibility with Project Browser Active .
Note also that Project browser seems to cache the logo images and ✨ Allow clearing of cached data from Project Browser sources Active seems to be a related issue to that.
I think I hit a bit similar issue today when I was testing the Drupal CMS RC2.
The first one was related to a contrib module logo.png. Yesterday I switched the module default branch from 1.1.x to 1.2.x. The 1.2.x branch had the new logo.png. Drupal.org module page was able to pick up the new logo.png after the cronjob in Drupal.org was ran. But I wasn't able to get the logo.png refreshed in Drupal CMS. At the end I was able to solve this by uninstalling and installing the Project Browser again which most likely also cleared the key/value storage.
The second issue was a bit more interesting. Basically what happened is we had a 1.1.1 release of the contrib module and that installed fine through Project browser. Today I released a new stable 1.2.0 version of the module. This stable version installed fine through Composer but when I tried to install the module through Project browser, it gave me an error that there is no stable release 1.2.0 available. This made me wonder if Project browser also saves the available releases information in the storage? Unfortunately I wasn't able to capture that error and it's now a bit hard to reproduce it without making a new release for the module. But again this new version installed fine after the uninstall & install steps for PB.
The logo problem is a small nuance but I wonder if the release problem might affect less technical users wanting to install and try out modules? It might be a tall order to make some less technical person understand that they need to clear some cache to get rid of the error. That being said, it might be that I just hit some other Project browser related issue when installng the new version but thought this might still be valuable information.
This is now fixed and will be released in 1.2.0.
This needs a new review because I made more changes to the project readme.
This is now merged.
This looks green now and ok to go.
This is now changed and looks good.
MR looks good.
Tested the general functionality manually and it seems to work fine except for the one PHP error.
Marking this as Needs work because it seems to give the PHP error if you install the new code but don't save the module settings.
Merge request is updated and pipelines seem to be passing. This still needs manual review.
Merge requests are updated. This still needs review.
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.