The main heading is "Governance of php-tuf/composer-integration and php-tuf/php-tuf", but the text later mentions "all three" and "php-tuf/php-tuf, php-tuf/composer-stager, and `composer-integration'". Do we need to update the main title to include all three projects?
Also added some minor style fix to the MR.
To give an update here regarding my previous comment #8 - that comment was relevant until D7 EOL (or until all D7 issues were hidden on d.o.). D7ES programs use their own systems or s.d.o. queue (where components are not same as on d.o.). So I think we probably do not need D7 components anymore (if there is no need for them for Gitlab migration or other technical reasons).
We recently added General committers to the maintainers roles list. Should we add also a separate section "Leadership Team: General committer" for this role to the "Who are the Drupal core maintainers, and what do they do?" section?
There is also a wrong indentation of some bullets in Leadership Team: Core Committers and Topic Maintainers , so updated that in the MR.
I created a follow-up issue to allow all contributors to change labels in Gitlab: 📌 Allow changing GitLab issues labels for all contributors Active .
Seems like there is a lot of IDEs mentioned in the MR (phpstorm, netbeans, eclipse, ..), but I do not see VScode folder excluded. We should probably add that as well (.vscode/). Not sure if any other popular IDE is missing though. Thanks!
Could this be a feature request for Drupal Core instead? Or any reason why to make this improvement only in Drupal CMS?
Seems like I forgot to change the status.
I have also updated the IS to reflect the new approach with replacing colons by underscores as proposed in #8.
@hatuhay Would be great to post at least a short explanation, how is this works as designed. I cannot see any reason why these hardcoded paths are a correct solution here. Themes can be installed in any directory inside /themes/. I am reopening this. Thanks!
Thanks for committing this, @hatuhay! I am curious if the status was accidentally changed to Closed (outdated), instead of Fixed?
Thanks for working on this!
I got the same as #9 but resolved it by uninstalling the Toolbar module. Then you also will want to enable Navigation.
Is it enough just to document that this is needed if you are looking to switch your site over?
I think that it was discussed earlier that Gin will support Toolbar (only the one default position) until it is removed from core. So I think we should fix that error instead of forcing users to use Navigation.
FWIW, there was already discussion about the "By" vs "Co-authored-by" in comments #7 to #37 and "Co-authored-by" was selected. I personally prefer "By", so +1, but I think we are getting in circles here.
Re #20 and #31 - I think this issue should be fixed regardless of the 🐛 Don't display OEmbed error to anonymous visitors when resource stops being available Needs work , because the error should not be visible to anonymous users. The other issue's MR does not fix the message displayed for anonymous users, so let's keep both issues open.
@marcus_johansson I am curious, why this has been closed, if it was not fixed in 1.x branch and it is still not fixed/rewritten/removed in 2.x branch? Why was not the issue version field updated to the 2.x branch and left open until it is actually resolved?
We invested significant time testing the AI module(s) in Drupal CMS prior to, or around the initial Drupal CMS release. Closing this issue (not sure if there are also others with the same outcome) as “outdated” now feels somewhat discouraging (if done without proper resolution and attribution). It just does not seem right to me. Thanks!
I still think that having common statuses for each project in the Drupal ecosystem is a good thing and it good also for contributors, because they do not need to study specific docs / workflows for each project and can just use one global docs like this https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... → everywhere. That is consistent and predictable. Imagine that some project will delete all status labels and introduce some weird custom labels. I am not seeing how that could be better.
The plugins are not considered a public API → , but I am curious if we should deprecate it first as per https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... → anyway? It is also a public property which could probably be used in some contrib modules (even though I have not found any evidence for that so far). Thanks!
Thanks for working on this.
I tested the MR manually and it fixes the problem. I have one question though - why the machine name cannot end with a dot? I have not experienced any issues when a dot was a last character, only when the dot was a first character. Can we add additional explanation to the IS, why we are removing the dot in the end, or sanitize only the dot at the beginning? The title also suggest that we are sanitizing only a dot at the beginning. Thanks!
Thanks for working on this.
I reviewed this issue and MR and I personally think, that we should not update these hardcoded tables in the current state, but instead we should try to convert them to views - which would give us much more options to customize them (also for site owners themself). I found that such issue already existed: ✨ Convert "Top 'access denied' errors.", "Top 'page not found' errors" and Top search phrases to views Needs work .
Regarding this issue - since it is adding a new column to the table, I think that is an UI change and as per Usability core gate → it needs screenshots in the IS and probably also a feedback from the UX Team about the new column title. There is also another similar issue to update the other column title here: 📌 Rename the table headers on the watchdog 403/404 controllers Postponed: needs info
I am postponing this issue on ✨ Convert "Top 'access denied' errors.", "Top 'page not found' errors" and Top search phrases to views Needs work . Thanks!
Adding a related issue. I think that if we convert these pages to views, then we will have much broader options how to customize/modify it (also for sites admins themself). Not saing this is blocked by that related issue though.
I agree with @ressa and reopening this feature request.
There is another issue to add additional column to the listing ( ✨ Include most recent date in page not found and access denied reports Active ), but I am not sure we should be doing that, given it is a static hardcoded table, without any pager or other possible customization options for site admins. Instead we should consider creating these pages as views, which seems to be a correct approach - then, almost any customization will be possible, even without additional core changes.
It does not seems to be hard to create a similarly working view - for an example view for "top 404 pages" see the config I attached (it is an export of the view we have used when replaced the old pages). It is not fully complete to be committed as is, but it can serve as some starting point for this issue.
Thanks!
Thanks for working on this!
I am curious if we need to run daily jobs with all three PostgreSQL versions (16, 17, 18), given we run daily jobs only for one MySQL/MariaDB/SQLite versions? Maybe we could change the existing PostgreSQL 17 daily job to PostgreSQL 18, to have the default and latest versions tested daily and not add a third one?
For the reference, here is a link to the original security issue: https://security.drupal.org/node/70884 (will work only for Drupal Security Team members) - the token lenght was discussed very briefly.
Thanks for working on this.
Currently the token is long 8 characters. The proposed solution will extend it to 40+ characters and will change URLs on existing images derivatives on sites. It can cause issues, because as far as I know, for example Google indexes the full URL of images, including query string (see for example this article).
It would be great if we can get some numbers about the probability to effectivelly bruteforce the current implementation to see, if there is a real chance of doing that and that it is really beneficial for sites to increase it/increase it that much (for example when considering a password length, from a specific point, there is minimal to no benefit having it longer, as it would mean practically no extra security in the current real world conditions). I think we should not make the token longer as really needed.
Moving back to NR to discuss it a bit more. Thanks!
Thanks for working on this.
I think that the proposed change can cause issues on some sites. As per https://developers.google.com/search/docs/crawling-indexing/robots/robot... , the Disallow: /search will match anything starting with /search, so it has a potential to also block any content with URL alias starting with that string (for example a news with an alias of /search-for-donations, etc.). Based on this, moving back to NW.
Also this is probably more like a Feature request than a Bug, given there is no issue with the core itself.
Once this is ready, I am curious if we can see a migrated test project somewhere (even in the staging/test env), to help with testing before doing the first production migration of the DA projects? That could probably help identify issues before doing first irreversible production migrations.
And should this issue 🌱 Using GitLab labels for issues on Drupal projects Active be a blocker too, since we probably need that decided before the first migration? Thanks!
Personally, I agree that keeping old NIDs as GitLab IDs seems more beneficial than keeping authors of issues. Great we can solve it at least this way.
Looking at my earlier comment #22 (and #23) - is there a decision whether we want to keep/migrate also the comments redirects? Seems like there are at least two places in 11.x branch (1, 2), where such a link is used. Not sure how many are used in issues' comments itself (can we check it in the database?).
Also have another question - currently the redirects on i/NID and links on node/NID are universal and are working even without knowledge of the specific project, in which the issue is created. I suppose that if we move an issue to a different GitLab project after the migration, the redirect from node/NID would still work to a correct (new) GitLab project?
And speaking about this (but this question is not related to the migration itself), it seems like that for all new issues created in the GitLab after the migration, we will loose the ability to find such issue just by entering the ID somewhere (without knowledge of the specific project)? Presumably, this is also because issues in GitLab have separate numbering in each project?
Thanks!
In D7 version of the module, there was:
$menu_item['xmlsitemap']['access'] = $menu_item['access'] && !$menu_item['external'] && !$menu_item['hidden'];
In the modern Drupal version, there is ony:
$entity->xmlsitemap['access'] = $loc && $access;
So yes, it seems like the module is not checking if the menu link is hidden/disabled. Only if user has access to the underlying link. I think that something like checking $entity->isEnabled() could work here.
I discussed this with @catch on committers Slack channel.
We both think that removing all messages without knowledge what else could be there is a bit risky. Even if the risk is small, it could affect some sites UX negatively, if any unrelated message will be removed accidentally. Another disadvantage is, what @ivnish asked in #91, that there is no way to remove just the one message, we need to change. Adding another message (and have two here) does not seems right.
I re-read the whole issue and it seems like, that the initial proposal was to add a description to the message textarea and only later it was proposed to add also a message on form submit. Based on this, the usability team proposed to remove the description and stick only with updating the submit message(s). However, the initial intention was to add only something simple like this: To display the maintenance mode message to all site visitors, flush the page cache on the Performance Settings page..
When we started mixing it with the submit message, it ended up as: The site is now in maintenance mode. The maintenance mode message will not show on already cached pages. To clear the cache, visit the Performance page.. Which is different from the original meaning, which was just to inform a user about caching. The current message also informs a user that the maintenance mode is turned on, which I think should not be in the scope of this issue, because we already have a similar message informing users about the active maintenance mode, see: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co... (Operating in maintenance mode...). This message is intentionally excluded from the system.site_maintenance_mode route (see
#184926: Incorrect message displayed when bringing a site back online →
). I do not think that we should add the information about the maintenance mode status back to the form, as users already see whether the checkbox is checked, or not. Or at least we should not do it in this issue.
Considering all this, I propose to return back to the original proposal - do not change submit message(s) and just add a simple and easy-to-read description to the message textarea (and do this only if page_cache module is enabled). Something like The message will not show on already cached pages without clearing the page cache.. I propose a bit different wording for the last part, so that we do not need to deal with the possible different permissions of the user (see #90.2). Another option discussed on a Slack was to put all information in the form, depending on the current state.
I am adding Needs usability review tag again and will ask for another opinion from the usability team based on these new findings.
Thanks!
My intention was not to hold this issue, but part of the RTBC issues review process → is evaluating the scope of the issue. I definitely think that changing the documentation for a same parameter in two functions directly related should be done in one issue, not in two issues, because it will create unnecessary work for maintainers a reviewers, see: https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett... →
If we take a look at \Drupal\Core\Mail\MailManagerInterface::mail() and \Drupal\Core\Mail\MailManager::doMail(), the documentation is mostly the same for each parameter. If we update only one of these functions, we will end up in inconsistent state.
Regarding a missing test - yes, there is a mention in the IS, that multiple addresses in the reply-to header works, but there is no proof for that (and also there is a comment #11, which states the opposite). Ideally, there should be either a test for this (which I have not found) or at least a confirmation (for example link to the documentation), that all implementations of \Drupal\Core\Mail\MailInterface::mail() supports it (at least the two implementations, which are supported by the core - PHP mail and Symfony mailer).
Thanks!
I have added one minor comment about the "read-only" vs "non-executable" wording.
Thanks for working on this.
I manually tested the MR since there are no tests to prove that the message is actually shown. It worked as expected. Anyway, I think we should extend the existing SiteMaintenanceTest to check if the code is working as expected and to prevent regressions in the future, especially if we are "blindly" removing original status message(s).
I also have some additional questions:
1. Aren't we worried that we could potentially remove more messages when using this code \Drupal::messenger()->deleteByType(MessengerInterface::TYPE_STATUS);? For example if there will be any custom messages included. We are not checking the actual message being removed.
2. Route system.site_maintenance_mode has permissions of administer site configuration+administer software updates, which means that if you have only administer software updates, you still have access to the maintenance mode form. But if you do not have administer site configuration permission, the newly added link to the Performance page will not work. Are we OK with keeping it there even if the user won't have access to it?
poker10 → created an issue.
Thanks for working on this.
It seems like the same parameter with a same docs (Optional email address to be used to answer.) is also in core/lib/Drupal/Core/Mail/MailManager.php (see: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...).
\Drupal\Core\Mail\MailManager::mail() passes the $reply parameter directly to the \Drupal\Core\Mail\MailManager::doMail(), so I suggest that we fix both occurrences here in this issue, as these are very closely related.
I briefly looked at the tests and it does not seems like that \Drupal\Tests\system\Kernel\Mail\MailTest::testFromAndReplyToHeader() is testing multiple emails in the reply-to header. So I suggest we extend the test and test the support for multiple email addresses, since we are putting it to the docs.
Thanks!
If we do not consider "co-authored" as a potential issue with copyright, as mentioned in #3540547-8: Add a UI to change "role" values for each user in the footer → , then for me either By, or Co-authored-by looks good. I would not add emails, as discussed above.
The updated text looks good, thanks!
The list of attendees looks correct, thanks!
benjifisher → credited poker10 → .
The list of attendees looks correct.
It looks like WP changed the algorithm to bcrypt in february and they seems to pre-hash passwords: https://core.trac.wordpress.org/changeset/59828 (line 2706).
If we go the way as proposed in the MR, then I think it will be better if we not fail silently in case the password will be longer - so agree with #14 that we should add at least some information for user.
benjifisher → credited poker10 → .
Indeed, we should add the ability to auto-detect in a follow-up
I still see the auto-detect feature in the MR, is that correct? Can we remove it for now, so that users are not confused how the values get there? If we would like to keep the auto-detect for now, I think we should bring back at least the descriptions to explain, that values were auto-detected and also mention the path which was autodetected. This can also help in case someone moves the site - then there will be the actual auto-detected link in the description and users can use it if needed (they will not need to search it manually). Anyway, the auto-detect feature is not mentioned in the IS in the proposed resolution.
In #36.4 I posted a bug on windows. Since we did not drop windows support yet (#3436617), is this worth exploring further? For example on my testing windows, composer was detected, but rsync not and I was stuck, because the auto-detected composer value did not pass the validation on the submit.
This seems like a (significant) UI change, so I think that even without an option to reset, the current approach still needs to be approved as per Usability core gate →
Based on these, moving this back to NR. Thanks for all the work here!
But if we include the module it eliminates the module selection process by the site template creator and consolidates usage to that module.
I do not think it is a good idea to explicitly select a specific module from a set of similar modules, unless Drupal CMS needs it. We have a big ecosystem of modules and maintainers put a big effort in maintaining these, so we should not make choices for our users, if we do not need (speaking generally, not about this specific case, where @svendecabooter is the maintainer of RoleAssign).
If you download and extract it, the vendor directory contains mostly empty folders, so this looks like a bug.
Wrong branch was selected when creating the MR. Updated that and moving to NR.
poker10 → changed the visibility of the branch 3529392-frontpage-settings to hidden.
quietone → credited poker10 → .
Thanks for reporting and working on this. There is a lot of unrelated changes in the MR because it targets 8.x-1.x branch. We need to have MR against 2.x branch. Also please keep open only one MR, as currently it is not clear, which one is ready for review (!63 or !64).
Thanks for reporting this.
Valid values as per specification (https://www.sitemaps.org/protocol.html) are:
always
hourly
daily
weekly
monthly
yearly
never
There is no "quarterly". Closing this as won't fix.
Thanks for reporting and working on this. There is a lot of unrelated changes in the MR because it targets 8.x-1.x branch. We need to have MR against 2.x branch.
poker10 → created an issue.
This seems to be a related issue: 🐛 Not checking "Allow multiple values" in contextual-filter, with PostgreSQL, results SQL error Active
But the Related blog posts display in the Blog view does not have Allow multiple values checked in the Content: Tags (Default: Taxonomy term ID from URL) contextual filter. Once the option is checked, the error is gone. So I think this can be fixed in Drupal CMS as well with adjusting the view config - if we set Allow multiple values by default.
@hestenet I am curious if there has been any progress with this issue? Thanks!
The list of attendees looks good, thanks!
Adding attendees according to the DST private notes.