Fixed as we did not port the mimetype guesser to 4.x yet, nothing to port.
May not be necessary if Core merges the requested changes to use php basename() instead.
At the moment the Drupal Module is currently primarily bringing in the 3rd party library in a manner that interface with Drupal's common API's. Many of these methods are still 'low level' and require knowledge of how to format Vault API messages. The VAULT HTTP API documentation is a useful reference. https://developer.hashicorp.com/vault/api-docs
The vault_key_kv module may provide some assistance of sample code to help (note: this is written for KV2 storage)
https://git.drupalcode.org/project/vault_key_kv/-/blob/2.x/src/Plugin/Ke...
$this->vaultClient->write($path, [$key => $value]);
I assume for this simplistic example that $path is a KV storage engine, the question is if it is a KV1 or KV2, the format of the submitted data depends upon this (KV1 it is a direct key/value pair while in the case of KV2 key should under a 'data' key )
KV1 submit format
KV2 submit format
Addtionaly care must be taken regarding / placement in $path to ensure path is correctly formatted with the base path already provided.
We appear to be missing documentation on this, a developer can set the vault.logger.level container parameter to debug to enable additional logging that may provide additional details. I will create a followup issue for that documentation to be added.
-c ./core/phpunit.xml.dist
I would suggest to attempt running without the core config to see if this helps your local testing.
Is this perhaps for a diffrent version of PHPUnit? We do us PHPUnit 10 currently with I belive Drupal Core 11.2 bumping us to 11.2 once GitLab templates updates the target core version.
We intentionally use --no-configuration in GitLabCi to resolve other issues that core is excessively strict about (such as 3rd party calling deprecated code) that do not apply to contrib and are generally considered the responsibility of PHPStan (core runs L2, we run L9)
Since these were written I did learn there is a method (register translation service in global container mock) to allow string casts to work which would have possibly allowed me to compare straight strings instead of the translatable object to allow for simpler comparisons.
We do need to convert to static as I believe it is either PHPUnit 11.5 or 12 that will make that mandatory (this one was missed somewhere in the process) .
Remaining comment of failures from #5 would need to investigate. I do believe I have seen some asserts will be changing location in PHPUnit 12.
I see the following in the test run window:
OK, but there were issues!
Tests: 197, Assertions: 1648, Warnings: 1, PHPUnit Deprecations: 12.IIRC core config fail on deprecation while we would desire not to (items that will need to be changed in the future)
Filling in a bit more info:
In ✨ Provide an alter for user settings Active I noted the status field was known to 'get out of sync'. I belive I partially was confusing a diffrent scenario where a plugin is listed as configured when isReady() will return false. However it is also likely this code may have caused some confusion in past where we see STATUS=1 with no plugins configured.
In
✨
Provide an alter for user settings
Active
we found that when saving TFA skip count for a user that has not configured TFA (would occur where the user has a required role and has not yet provisioned a token) that the the user.data is set to status=1 indicating TFA is enabled.
This was first noted by @msypes in https://www.drupal.org/project/tfa/issues/2958950#comment-14922059 ✨ Provide TFA enabled status field via views Fixed
This is questionable since the user has not actually configured TFA, however one could also argue that TFA is enabled due to the fact 'TFA is enforcing a skip count' and that the user is 'within policy' at the point even though no token is required.
This caused the followup issue of 🐛 Views "TFA status" field wrong output Active to be opened.
First question we need to ask is do we want skipped counts being set without enabled plugins to be considered 'TFA Enabled' or 'TFA Disabled' for the backend status field as that impacts our operations choice.
Related, we need to analyze how this would impact the code if we choose disabled, will enforcement of users change, if so in what way and will it be acceptable.
Currently there is a heavy usage of isReady() against all plugins which may render any immediate concern about this value of little importance, however long term we do want to get to a state where we have a reliable 'is TFA enabled" indicator which we need to consider for this code that 'enabled' may be the appropriate value
Also related, since this is a default value, is what impact this has on 'fail secure' operations of the code as we work towards 🌱 [META] Convert fail-open code executions to fail-secure alternatives Active . At the moment it sis also likely backed up by isReady() checks however those are heavy operational cost and ideally we would eventually get away from them as this has caused other features such as ✨ Force user to setup TFA when required and there are no remaining skips Needs work to be harder to implement.
On a cursory read this appears it may be a duplicate of
🐛
Full Setup not working on 8.x-1.7
Active
/
🐛
One setup step remaining, two QR Code Scans required
Active
.
Can you confirm those issues are not related?
Additionally:
D.O. has migrated away from patch files.
In order for tests to run we require all submissions to be in the form of a Merge Request.
More details may be found at: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →
When a user has disable own tfa but not administer tfa for other users while viewing another user's TFA page
I'm not able to duplicate having access to another users TFA page unless they have the administer tfa or other users permission.
If you are able to duplicate this scenario it should be rasied as a private security issue for further discussion.
When a user has administer tfa for other users but not disable own tfa while viewing own TFA page
I am able to duplicate this, and attached patch does appear to remove the link.
As noted in IS access is blocked when clicking on the link making the change purely cosmetic.
was I supposed to tag this somehow?
Currently under Drupal.org issues are generally marked 'needs review' when ready, although I imagine when D.O. move's to GitLab issues it may just be the presence of a non-draft MR.
Just wanted to validate you were not working on any concerns before I proceed with testing
No conflict rebased on orgin/11.x
Deprecation set to D12.0
Deprecation added for constructor argument
Original related CR referenced to this issue
We do have integrity checks ensuring the commit emails are always synced correctly.
The current value is __private which is a GitLab internal value.
My understanding is from an API standpoint this this value is not the same as syncing the Primary email address from KeyCloak (which is currently set as my Primary email address in GitLab).
This value is set under Profile > Main Settings > Commit Email. There should be nothing to/from D.O. or KeyCloak should there?
This is the first request for this update we’ve had.
I will note I have had the request to be able to make this change on my own open since March of 2022. Having waited 3 years I felt it was reasonable to request infra to make these changes on my behalf.
The desire is to have new commits be able to be linked to my profile in GitHub and GitLab which requires it to be an email address that can pass validation to prove ownership.
Attached photo from GitLab showing the necessity for this.
It is my understanding G.D.O. has the same requirement preventing GitHub and GitLab addresses from being added as secondary addresses.
I additionally desire to have my commits attributed to a real email address as part of managing my copyright to each commit to provide a long term reachable email address.
It is worth considering SA-CONTRIB-2025-085 would not have been possible if the codes were hashed using only a 1-way algorithm.
It is worth referencing NIST-800-63b rev3 section 5.1.2.2
Verifiers SHALL store look-up secrets in a form that is resistant to offline attacks. Look-up secrets having at least 112 bits of entropy SHALL be hashed with an approved one-way function as described in Section 5.1.1.2. Look-up secrets with fewer than 112 bits of entropy SHALL be salted and hashed using a suitable one-way key derivation function, also described in Section 5.1.1.2. The salt value SHALL be at least 32 in bits in length and arbitrarily chosen so as to minimize salt value collisions among stored hashes. Both the salt value and the resulting hash SHALL be stored for each look-up
(The draft of version 4 is even more strict and calls for a password hashing algorithm for those codes less than 112bit’s entropy)
Given above I would suggest v2 implement this request as mandatory for v2 with non reversible storage of recovery codes.
Thank you for the work on this one!
Committed to 2.x dev, not sure how much of this we want to backport to 8.x-1.x, we still have MR!144 open, that is the 'first pass' solution, we could go with that, or we can look at if we should backport MR!145 to 8.x-1.x.
I noted in the thread that I wonder if we should backport with translated string changes. I will however note they are admin only and realistically MR!144 added new strings too so this wouldn't seem to be significantly different.
Would be a small UI layout change, we haven't really established a policy yet regarding how much UI can change, this does appear to be rather non-disruptive overall.
Flagging as need backport for discussions on what (if anything) makes it back to 8.x-1.x.
cmlara → created an issue.
I believe this is actually a duplicate of 🐛 Local stream wrappers break when directory is missing Needs work as its the inherit root cause is that a configured directory for the streamWrapper did not exist and core did not attempt to validate it existed before usage which caused cascading failures that should reasonably have been predicted to occur.
Back to NR after the branch was rebased.
Never received a merge conflict email, did this actually have a conflict or was this a bot FP?
NW per MR comments.
NW per MR threads.
Should add a test with this to validate that token generation is actually being called and processed.
@jvollebregt-swis do you consider this ready for review or is there other work still to be done?
Re #11 and #12: Event names sound good and plan to only deal with TFA Enabled/Disabled changes in this issue deferring plugins to a separate if requested by community.
Regarding layout: I would suggest we use the Event Class Name method rather than "string name" method. Given its design should make code management easier long term.
The Class Name method has no need for a matching map name or attempting to determine the correct string to match to the event, the class itself is the definition and dispatch can essentially be $eventDispatcherInterface->dispatch(new TheEventClass());
Bot is only testing MR!12525.
That said it may be legitimate tooling failure as it would change the tooling requirements.
Back to NR for MR!12510, if it weren’t for the required FM sign off I would call it RTBC.
Do we need the user data to be passed forward in the event?
That data really is internal, it has no formal API between plugins or even necessarily between patch releases.
In TFA we (generally) know when we change the user in TfaSetupForm.
I was thinking we could be emit either a "TfaStateChange"(enabled/disabled) (or separate events for either case such as TFAEnabled/TfaDisabled) and equivalent for Plugins (TfaPluginStateChange or individual TfaPluginEnabled/TfaPluginDisabled) events (descriptive examples not intended to indicate what events must be called). May be possible to combine the general and plugin events into one, care should be taken on how well they can be used by consumers such as ECA/Rules.
Only reason I can think of to do this in TfaUserDataTrait is if we are worried about a module other than TFA making changes to the data, however in that case it really needs to be a decorator on the core user.data service and not in the TfaUserDataTrait and it could be said to not be indicative of TFA making the change.
I suspect ✨ User account should be marked as updated when TFA is updated Active may end up being a dedicated TFA event (as I’m not aware of a specific user save event we could leverage) however nothing about that has been formally said in that issue.
If you want to rescope this issue: I can see TFA emitting an Event when we enable/disable TFA for a user or a specific plugins is provisioned for a user as making a significant amount of sense without using the hook system (especially not using alter hooks).
✨ Create metapackage of (loose) contrib test dependencies Active touches on the comments from #7 and #10 in that we ultimately don't have a good package for "developing contrib" vs "developing core" to pull dependencies. For contrib the phpstan constraint really is not needed.
Long term I do suggest Core consider if they even need a package for "developing core" package. The core composer.json should be able to load everything a developer needs to test core meaning there really is only a need for listing what is needed by contrib to use Core as a test platform.
To me: Visually MR!12510 change looks fine.
Biggest risk of doing this is some downstream dependency lists PHPStan (or related modules) as a regular dependency instead of a dev dependency forcing back to ^1, or some other sub-dependency of PHPStan (or related modules) is somehow forced back to a version only supported on older releases. Both of those will however likely fail the jobs (baseline not matching) so will be know very quickly, and if that does occur the ^2 constraint alone would fail the Composer stage meaning both scenarios require interaction by the Core team to resolve. MR!12525 would suffer the same risk.
Essentially the risk to core is minimal, and would generally just show up at a different stage.
Linking 🐛 Can't update to 11.2.0 Active for cross-issue visibility (on Slack there was originally some discussion of using that issue to handle this concern).
It would be a good option to alter user data before saving
can you provide a real life scenario of this being useful?
Everything in the user data that I can think of (TFA metadata, seeds) is essentially self owned/internal (to a plug-in) data that shouldn’t be touched by external actors.
waiting for the update compatible with drupal 11.2
📌 Allow PHPStan ^2 Active is the current blocker, as the tests runs in this issue indicated it now identifies numerous errors (most legitimate however had previously been baseline ignored, and at least one bug in phpstan-drupal). It is only after working through a significant portion of the reported errors that it became apparent that some will not be 'dual fixable' and the discussions around that have been started.
There is talk that core may roll back its demand for ^2 however that, has of yet, not occurred. End of the day we want/need to know we can expect to have clean testing runs before we formally release support for ^11.2.
I can suggest if site owners want to help avoid this in the future a few items they can consider:
Encourage Core to make only bugfix changes after Beta 1.
Encourage Core to ensure major changes are committed to the repo earlier in the development cycle.
Encourage Core to extend the Beta/RC window lengths to allow contirb more time to update before the production release is made available.
Encourage Core to withhold releases when there is any doubt expressed about the suitability of the release (the PHPStan issues were identified in Slack with a Release Manager however the 11.2.0 release was pushed ahead as it was a week behind schedule).
Encourage Core to make more of its API covered by BC Policy (the primary reason we list 'by minor' rather than 'by major')
With the above said, we are currently only 7 days into the lifecycle of a release that has 1 year until its EOL and have ~6 months until 11.1 goes EOL. Would I prefer this was released the same day (or sooner) than Core, yes, however if core makes last minute major decisiosn that impact contrib there is little to nothing we can do except work through it.
The irony of this issue I actually looked into adding add PHPStan ^2 support in November however Drupal Core's requirements prevented it at the time and it appeared we would be good until ^12 based on discussions with other core devs.
Considering the above document is not a policy is there anything actually stopping the Project Application Queue from testing the proposed process for 3-6 months and seeing how it works?
There is discussion on Slack that 🐛 Can't update to 11.2.0 Active may be re-purposed to include rolling back 📌 Update PHPStan to 2.1.17 Active as part of a hotfix release.
@dimitriskr
You may want to comment on 🌱 More flexible language for git vetted status for co-maintainers of existing projects Active . This application came up in relationship to that issues today.
I'm not sure where the question regarding are these specific documents are policy have been answered unless @avpaderno is referring to discussion that other documents (Project Adoption) which has been stated are not policy without firm statements regarding other documents on D.O. Perhaps the intention is to mean that no documents on D.O. are policy (in which case how can they be used as evidence to refuse a request?).
I will note the use of "Privileged User" was intentionally chosen to identify users with enhanced privileges not available to the average user, and to align with the D.O. Code of Conduct to not question the motives of specific individuals, instead questioning the process and its implementation. @avpaderno was previously made aware of the terms NIST definition on Slack.
@avpaderno For those of us who are not D.O. Privileged Users, since there has been discussion on other pages with the same concern, can you confirm which of the links in #10 are policy documents (if any) vs just general documentation?
@avpaderno I believe the point of #7 and #9 is that the D.O. Privileged Users are required by the Drupal Code of Conduct (regarding collaborative and abusive behavior) to apply gates equally to all users.
In the linked issue the D.O. Privileged User repeatedly refused to agree that the code did not demonstrate their individual skill, non-privileged user thus must assume that such little edits is acceptable and that they can expect similar treatment on other issues.
Note: I have not read the code in full, I have simply skimmed the related MR which appears to contain a similar level of work as the linked issue that was repeatedly considered 'acceptable'
My apologies, I had thought I had submitted my response on this issue previously.
Are you interested in contacting the upstream maintainer there
My entire knowledge of this reported vulnerability is based off the vulnogram, ideally someone who knows more or has been involved in working on the repair to the Webform Multi File Upload module would do so as they would likely be better informed.
it seems we could issue a CVE if the upstream maintainer is not interested in doing that
.
Possibly there are other parts of the CNA rules to consider as well:
4.2.1.1 The CNA with the most appropriate scope MUST be contacted and given the first opportunity to assign.
4.2.1.2 also comes into play that if the most appropriate scope refuses the Root CNA must make a determination and direct a CNA to issue.
then an appropriate Root MUST make a Vulnerability determination. If the Root determines that one or more Vulnerabilities exist, the Root MUST direct a CNA-LR or another CNA with appropriate scope to assign as quickly as possible and no later than 72 hours after becoming aware of the first refusal.
Question is, who is most appropriate scope?
4.2.2.1.3 scope determination is relevant to review.
Drupal.org CNA scope is “All projects hosted under drupal.org, including End of Life (EOL) code.”
What is the interpretation of hosted? Is it being the official development platform, or is redistributing also hosting?
How integrated is the code in the Drupal module (is it line for line, or is it a fork that deviated some time in the past and could legitimately called its own project)?
Is there another CNA with better scope?
I’ll note the goal behind the process is that for any single vulnerability there is only a single CVE to refer to it no matter who re-distributes the code and that any program that contains the flaw references that CVE. All of the above questions are above my current knowledge on the module and how the code was distributed.
I should note Drupal may not technically be able to answer "yes" to "Publicly known vulnerabilities fixed"
Drupal Core likely breached the timeline with CVE-2024-45440 which took untiill at least Decmeber 4th to commit to the D7 repo.
There is still an open second part in 🐛 Maintenance Pages leak sensitive environment information - pt2 Active .
If we ignore that for a moment, we should also consider that there are a large number of issues that are public that may qualify for a CVE if escalate through the CNA dispute process. At the moment they would not technically disqualify Drupal, however is it in the best interest of the project to assert a strong security focus if it has not resolved those open issues?
@dimitriskr
You may want to look at 📌 [1.0.0] Accessibility tools Needs review to see if it can provide you any solid points of argument. That was an application where a project forked a 3rd party and made minimal changes to itself. It may provide you the evidence you need that you have made 'sufficient' changes to apply..
Call to method Drupal\Core\Entity\Query\QueryInterface::accessCheck() with false will always evaluate to true.
is related to https://github.com/mglaman/phpstan-drupal/issues/825
to make a check against the database to see if any images are using the wrapper in the files_managed table.
Image styles may be generated for un-managed files as well (albeit not common), I don't see the files_managed table as a viable solution.
We should definitely see if we can at least warn users. I just don't know where/how that could obe done
We have had this subject come up for other issues in the past. Documentation has to start somewhere, this issue is as good as any other to do so, stick it somewhere in the README, or some "deployment guide" for Site Owners to note that '/styles/' is a reserved path on all streamWrappers.
Potentially a feature to be able to select where images styles may be located could be a solution.
✨ Split ImageStyle into the config entity and a separate event-based image processing service Needs work would be the best for this, it would allow solutions for numerous concerns. The new FlushEvent it proposes likely could allow this to be handled per streamWrapper by later changing the streamWrapper spec to require each streamWrapper provider listen and perform the flush as needed.
The relevant config is controlled by the Core team not infra.
Noted in Slack this sounds exactly like the cspell issue previously encountered in 🐛 Autoresolve cspell GIT_DEPTH issue when diff fails Fixed .
The --audit flag is enabled for every package manager operation - this includes unattended automatic updates which could be run via a cli cron job.
I was reading #4 and following as suggesting that audits NOT be a part of the package manager operation (wasn't aware flag explicitly was there vs the implicit composer use of it by default).
so "silently allowing a 'known vulnerable' package to be installed" is the status quo.
Understood on that being current and IMO bug on its own that it should be handled rather than silently allowed.
I think a new issue to add an explicit composer audit and show the results would do a lot more here.
There can be some value in this. Assuming that an audit call is added as part of a transaction that prevents a vulnerable package from being installed beyond just a 'sandbox' and transferred to 'production' that would likely be acceptable.
I would suggest that should be done before passing the audit ignore flag which may cause a successful return from Composer (in other words, postpone this issue).
Note: I'm not familiar with Project Browsers internals. I see comments such as 'sandbox' above and assume its doing a standard A/B on disk with a staged swap over only after passing basic checks such as composer success.
Lets keep this one as the common issue/meta, especially since It is already being linked to.
As I mentioned I've went ahead and changed scope a bit.
We should avoid the scope creep, Changing the testing environment (not required to test this, see https://git.drupalcode.org/issue/s3fs-3528246/-/pipelines where manual runs were used to determine the information above)(this was a key feature of how gitlab_templates was designed to replicate older drupal_ci, although I admit many developers have never learned about it) and adding support for PHPStan 2.0 are indeed both likely reserved to their own issues. (I haven't even looked at the 2.x problem yet to see how it impacts our ability to run against ^10 and ^11)
Smaller the changeset the quicker I can approve and release a targeted 10.5 instead of having to review everything related to ^11.2 before push.
I've simplified version constraints
Did have simplifying slated into this issue, although I will note I saw <10.6 and thought "wait 10.6 doesn't exist yet" before noticing the lack of an = sign. Technically its as equivalent to what was initially proposed in the MR to simplify, I'm not sure which does a better job conveying the logic, open to community feedback.
PS I would suggest dropping support of D8 and D9 altogether, as well as PHP7 .
Discussed elsewhere, short version my opinion is that (under semver) these all require a major release. Technically we could get away with it as we are not semver yet however there is no need for us to not treat 8.x-3.x as equivalent to 3.x.0 as much as possible. I absolutely do NOT suggest running it on these old of versions unless you are backporting security critical changes (and I still suggest upgrading). I mentioned elsewhere as it relates do drupal/core we are writing to an API spec (which never really goes EOL) not specific versions of Core (which do go EOL).
It's not being tested anyway, so is it still working ?
While we may not be testing it on the weekly test run (gitlab_templates has been poor on maintaining BC leading to fears of adding it) I do run manual tests back as far as PHP 7.4 Drupal:^9.5 periodically, especially when major code changes take place.
drupal-gitlab-local --variable='_TARGET_CORE=^9.5' --variable='PHP_VERSION=7.4' --variable='PHP_IMAGE_VARIANT=apache' composer phpunit (this should be re-producible on GitLabCi with the same config)
OK (88 tests, 600 assertions)
Generating code coverage report in Cobertura XML format ... done [00:00.771]
Code Coverage Report:
2025-06-20 02:57:19
Summary:
Classes: 17.95% (7/39)
Methods: 37.87% (89/235)
Lines: 49.17% (2078/4226)
Eventually I will get back to work on 📌 Convert to Quasar Components Active (last I worked on it I had testing working back as far as ^8.9). Ideally this project would have proper MIN/MAX testing. We have accidentally breached our support as discussed in 📌 PHP7.0 support Active and we are waiting on drupal/coder to allow us to implement 📌 [PP-1] Create Gitlab job to test Minimum PHP Language syntax Postponed so our testing right now is indeed not perfect.
I want to re-afirm @tedbow comments from #11 Generally with testing you want to test "worst case" in this case that means Audit On as it will likely be enabled on sites.
could exit with exit code 1, even if the install was succesful. In a programmatic way, this will for sure create confusion
Arguably an error has occurred and package_manager does need to deal with it.
I would imagine we will never show the results of the audit directly in the composer UI, it would be an explicit operation,
Also I think we should skip the audit operation in regular package_manager operations too -
I can understand not displaying, however silently allowing a 'known vulnerable' package to be installed? To my knowledge php-tuf will not be replacement for composer audit(security advisory parsing).
Large commercial sites are much less likely to use Package Manager, they are going to run controlled labs with dev environments. Tooling like this is really an advantage for the SMB/Home User market. Both of these are signifcantly more likely to run in production. Home users have much less concern, the SMB market however, their insurance agent will look for any reason to reject a claim, intentionally ignoring/not processing AUDIT results may make a very good reason to reject.
Even just having the code on the server may be enough for an auditor to flag a concern. From an audit standpoint it would likely be better to encourage users to set the COMPOSER_NO_AUDIT environment variable through settings.php to disable processing so that it does not impact more secure sites and is not enabled by default.
Is #18 a formal Security Review or should this issue be flagged for that as well?
https://www.drupal.org/project/drupal/issues/3526142#comment-16151180 📌 Update PHPStan to 2.1.17 Active is also likely to impact D11.2 tests runs as we are configed
The issues in #4 for 10.5 were likely due to an out of date origin locally (missing the deprecation silence fixes).
📌 Update PHPStan to 2.1.17 Active may add some complications for 11.2 as core is now forcing PHPStan v2 which is causing us to download mismatched dependencies. Once core releases 11.2.1 I expect composer failures to block all testing.
- Locking drupal/core (11.2.0)
- Locking drupal/core-composer-scaffold (11.2.0)
- Locking drupal/core-dev (11.2.0-beta1)
Considering 10.5 was released a few days ago and we would be 'rushing' 11.2 if we forced it out today we might consider separating D10.5 into its own issue and have 11.2 be a followup a (hopefully) a few days later..
@kim.pepper Thanks for the quick review. Was typing up the following while your review came in:
This was actually not as embedded in core as I expected it to be.
Set deprecation for 11.3 removal in 13.0 based on (the not yet accepted) discussion in 📌 Defer disruptive 11.3 deprecations for removal until 13.0 Active out of concern it might be considered 'disruptive' (removing a method).
As an argument it is not disruptive: basename()is PHP native and works in PHP8.1 which is the minimum supported version for any D10.x release. A module could easily support ^10 || ^11 || ^12 with just the native function.
Above my position to make the final call on D12 vs D13 (willing to edit to D12 if it is approved).
This does impact a deprecation added in
🐛
Separate MIME type mapping from ExtensionMimeTypeGuesser
Needs work
the file_system was added to the ExtensionMimeTypeGuesser as optional in D11.2 and would have been required in D12 will now be removed in D11.3.
The Password Reset tests were handled in [##3392427] which was the primary hold on this issue for 2.x.
Manual rebase on head, with corrections for accidental rollback.
Until https://github.com/php-tuf/composer-integration/issues/128 is resolved, I have basically no capability to help with issues like this.
Given we had issues in #30 is there any background process running to routinely verify the integrity of the repository (and alert when integrity is breached) that could identify these issues prior to a report from a user and without need to resolve the issue in TUF?
Duplicate of currently open ✨ Suggestion to only show validation skip status when relevant Needs work
If I am not missing something to discuss, shall we close this at WAD?
The original request was to formally adopt a standard. That would mean this would still need a coding standards vote to adopt and update https://project.pages.drupalcode.org/coding_standards/ with new text. That would not be WAD, if nothing is to be formally adopted it would be "won't fix" which would be saying "each project may write markdown files however they want"
Various projects, including the Drupal governance and this project are moving documentation to GitLab pages.
I will note the coding standards and Drupal governance project pages are not using GFLM, since the pages are generated by mkdocs they must align to python markdown, which is is different from and in some cases incompatible with, GFLM. 🐛 Formatting of PUWG charter mkdocs output is wrong Active is a real world example where parsing differences between GFLM and Python Markdown.
In #10 and #23 i mentioned that
Adopting GLFM is equivalent to saying "The coding standard for PHP files is the PHP engine specification", nothing about unified writing style has been said other than "the parser must not error'.
Here are a few questions that are related to standardized writing of markdown from the first couple pages of the GFLM specifications, each is a decision to make, each choice is valid, and each option can be mixed in the same file (and in some cases even the same line) and still be valid GFLM:
- Underline or Hash style heading indicators
- Max line lengths (do we keep the old PHP 80 char limit or do we embrace unlimited line lengths which better align to markdown usage)
- Double underscores or double asterisk for bold
- Hyphens, asterisks, or underscores for horizontal rules
- Should ordered lists use the 'repeating same digit' method of numbering or should they manually number each item (so that it is readable in plaintext too)
This is just a fraction of the choices that would need to be made to unify the writing of markdown files under a common style.
Would be good to have a subsystem maintainer sign-off before any effort is put into code on this.
Pinging @kimb0 (Kim Pepper) on Slack.
Digging a bit more into the core side history of this, there there is/was apparently a locales issues.
https://bugs.php.net/bug.php?id=37738
It does appear this issue is no longer present in modern PHP versions, although it does exist for the entire PHP7 line.
https://3v4l.org/XcflK.
Perhaps we will want to copy the
FileSystem::basename()
as PHP7.1 is listed for S3FS supported environments.
Added notes to the file comment referring to core and this issue.
As a deeper summary:
The history on s3fs having initialized its own copy of the MimeTypeExtensionGuesser can be traced back to
#3259065: Circular reference detected for service "s3fsfileservice" with mime_type guesser →
. We were avoiding a circular reference after having added s3fsfileservice which decorates the core file_system service.
Some (many?) MimeType guessers make use of the FileSystemInterface::basename() method. In the past this has been via a global container call \Drupal::service('file_system') in-line, however as part of proper service injection these calls are slowly being removed.
The s3fsfileservice is somewhat unique with the fact it needs to know the mime type of a file as part of the Move/Copy operations to correctly set the headers from the S3 Bucket.
FileSystemInterface::basename() is unfortunately just in a poor location for this scenario.
To solve this our copy will use the native basename() function. FileSystem::basename() appears functionally identical to the PHP basename(). It would make no sense for a streamWrapper to be implemented in a manner where basename() does not follow the native format as such streamWrappers would be incompatible with normal PHP scripts. The entire purpose of streamWrappers is to provide program agnostic storage.
Ideally as a followup we would encourage core to deprecate/remove FileSystemInterface::basename() to allow us to return to operating with the Core instance of mime type guessing services.
The error as described was never able to be re-produced.
Testing against Drupal CMS fails.
mkdir -p /tmp/dcms
cd /tmp/dcms
/usr/bin/time composer create-project drupal/cms
cd cms
/usr/bin/time composer -vv update
composer config allow-plugins.php-tuf/composer-integration true
composer require php-tuf/composer-integration:dev-main --dev
mkdir tuf
curl -o tuf/packages.drupal.org.json https://packages.drupal.org/8/metadata/1.root.json
curl -o tuf/packagist-signed.drupalcode.org.json https://packagist-signed.drupalcode.org/metadata/1.root.json
composer tuf:protect https://packages.drupal.org/8
composer config repositories.drupal-core composer https://packagist-signed.drupalcode.org
composer tuf:protect https://packagist-signed.drupalcode.org
composer -vv update
> pre-file-download: Tuf\ComposerIntegration\Plugin->preFileDownload
In RootVerifier.php line 36:
[Tuf\Exception\Attack\RollbackAttackException]
Remote 'root' metadata version "$1" does not the expected version "$2"
Exception trace:
at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Metadata/Verifier/RootVerifier.php:36
Tuf\Metadata\Verifier\RootVerifier->checkRollbackAttack() at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Metadata/Verifier/RootVerifier.php:25
Tuf\Metadata\Verifier\RootVerifier->verify() at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Metadata/Verifier/UniversalVerifier.php:59
Tuf\Metadata\Verifier\UniversalVerifier->verify() at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Client/Updater.php:231
Tuf\Client\Updater->updateRoot() at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Client/Updater.php:152
Tuf\Client\Updater->refresh() at /tmp/dcms/cms/vendor/php-tuf/composer-integration/src/ComposerCompatibleUpdater.php:51
Tuf\ComposerIntegration\ComposerCompatibleUpdater->getLength() at /tmp/dcms/cms/vendor/php-tuf/composer-integration/src/TufValidatedComposerRepository.php:220
Tuf\ComposerIntegration\TufValidatedComposerRepository->prepareComposerMetadata() at /tmp/dcms/cms/vendor/php-tuf/composer-integration/src/Plugin.php:55
Tuf\ComposerIntegration\Plugin->preFileDownload() at /var/www/html/vendor/composer/composer/src/Composer/EventDispatcher/EventDispatcher.php:220
Composer\EventDispatcher\EventDispatcher->doDispatch() at /var/www/html/vendor/composer/composer/src/Composer/EventDispatcher/EventDispatcher.php:115
Composer\EventDispatcher\EventDispatcher->dispatch() at /var/www/html/vendor/composer/composer/src/Composer/Repository/ComposerRepository.php:1653
Composer\Repository\ComposerRepository->asyncFetchFile() at /var/www/html/vendor/composer/composer/src/Composer/Repository/ComposerRepository.php:1111
Composer\Repository\ComposerRepository->startCachedAsyncDownload() at /var/www/html/vendor/composer/composer/src/Composer/Repository/ComposerRepository.php:1039
Composer\Repository\ComposerRepository->loadAsyncPackages() at /var/www/html/vendor/composer/composer/src/Composer/Repository/ComposerRepository.php:540
Composer\Repository\ComposerRepository->loadPackages() at /var/www/html/vendor/composer/composer/src/Composer/DependencyResolver/PoolBuilder.php:429
Composer\DependencyResolver\PoolBuilder->loadPackagesMarkedForLoading() at /var/www/html/vendor/composer/composer/src/Composer/DependencyResolver/PoolBuilder.php:279
Composer\DependencyResolver\PoolBuilder->buildPool() at /var/www/html/vendor/composer/composer/src/Composer/Repository/RepositorySet.php:332
Composer\Repository\RepositorySet->createPool() at /var/www/html/vendor/composer/composer/src/Composer/Installer.php:501
Composer\Installer->doUpdate() at /var/www/html/vendor/composer/composer/src/Composer/Installer.php:298
Composer\Installer->run() at /var/www/html/vendor/composer/composer/src/Composer/Command/UpdateCommand.php:281
Composer\Command\UpdateCommand->execute() at /var/www/html/vendor/symfony/console/Command/Command.php:279
Symfony\Component\Console\Command\Command->run() at /var/www/html/vendor/symfony/console/Application.php:1076
Symfony\Component\Console\Application->doRunCommand() at /var/www/html/vendor/symfony/console/Application.php:342
Symfony\Component\Console\Application->doRun() at /var/www/html/vendor/composer/composer/src/Composer/Console/Application.php:396
Composer\Console\Application->doRun() at /var/www/html/vendor/symfony/console/Application.php:193
Symfony\Component\Console\Application->run() at /var/www/html/vendor/composer/composer/src/Composer/Console/Application.php:136
Composer\Console\Application->run() at /var/www/html/vendor/composer/composer/bin/composer:99
include() at /var/www/html/vendor/bin/composer:119
I belive Documentation is where this was intended for.
Related Slack Link: https://drupal.slack.com/archives/C0451JV7HRD/p1749754384761549
Wondering if the UI should become a tableselect like this instead as some of these descriptions are getting rather long. Perhaps this makes it easier to read?
That does appear to be easier to read and allow for a nice delineation between the role/description that is otherwise missing. I also like how the role is called out for both.
+1 from me (based on the screenshot) for the changes to be implemented.
Will want to backprot this to 8.x-1.x as well, even though the form is not likely to change we do not want to encounter this flaw in the future.
cmlara → created an issue.
It is a documentation guide written for project moderators to make sure the followed procedure is the same for every project
This is not a Site Moderator only guide. It is also used by the community by applicants to understand when and how they can apply.
This page lineage can be tracked to the "Getting involved - Dealing with unsupported (abandoned) projects " which was intended for community members. An archived copy can be seen at https://web.archive.org/web/20170717080153/https://www.drupal.org/node/2.... That node was deleted (twice) in 2022 by @avpaderno when they moved the page to its new location, see 🐛 Taking over unsupported abandoned projects has gone away Needs review for incident information.
for project moderators to make sure the followed procedure is the same for every project
If it does not cover all scenarios how can it ensure that the procedure is the same for every project?
I am asking for the documentation to be updated to include the scenario that occurred in 📌 Request to assume module maintenance Active that a user without the opt in permission was able to be added to a project that was already enrolled in the security advisory policy. That way a user without the permission knows there is a chance they they can be approved (although not required to be approved) (or if that application wasn't suppose to be approved it would be wise for the project ownership queue to cleanup the issue).
As an aside since the contention on "will" is a large part of our discussion (I suspect English as a Second Langue is involved here):
I guess nobody would complain about using I will come to live in Italy. saying the sentence must make clear that coming to live in Italy is optional.
You have used this in the past to explain will as 'optional' however in this sentence I would contend the normal interpreted to mean the person writing the statement saying that in the future (either as wish/desire/goal or known truth) their home shall be Italy. Traditionally in this phrasing it is intended to indicate a future certainty. If there is no certainty it would be written "I want to live in Italy" to express the desire without the intention to ensure it occurs (not a life goal).
This appears to needs an IS update to have those running the composer steps gather memory (and time if it really took 10 minutes) usage to be included in reports.
Should have set back to active after providing feedback that
Policy “is optional” is based on actions and statements of Project Ownership admins.
Core now has a way to highlight when config has been overriden:
Interesting, I knew that config_target would allow usage of Core validation of config, and IIRC it could be used to render the config forms (we are not ready for that in s3fs) however was not aware it was now the solution for indicating overrides.
Could be useful to adopt.
📌 Increase minimum requierments for 2.x branch Active makes this issue much easier as we only need to support Drush13+ in the 2.x branc
Committed to Dev on 2.x, no backport due to changing the plugins architecture (generally all internal however API not well defined in 8.x-1.x)
Fixed, will not backport as it could impact 1.x plugins in unexpected ways.
As noted this is required in order to maintain SQL compatibility. The 4.x branch already has a fix for this deployed.
cmlara → changed the visibility of the branch 3501484-error-while-using to hidden.
Committed to dev. We will likely have a new release to add D11.2/D10.4 support in the near future.
Tagging for 4.x as we likely need this there eventually.
PHP Min is now committed at 7.1 allowing this to proceed.