Maybe this should be added to 2.x as well?
Thanks for your work here people ♥️🙏
Is there a core or contrib entity type to use to reproduce this?
This way it should be easy to add a test, which I think we need here
Awesome thanks ✌️🚀
That's a very valid reason. However, I want to offer another view at it.
I think running composer operations on live site, even though we do it in sandbox first, is potentially dangerous so I think being paranoid and trying to ensure setting up things the *right* way, as much as possible, is warranted.
Ideally one would of course not want to run composer on a live site. But we know that will be the case already for many, plus using package manager is in practice exactly that. But personally, I would for sure turn off auditing on a live site. The auditing should occur other places than on prod while building your site. But yeah, that ideal scenario is not what we are catering with package manager for sure :)
In addition, I would argue it's less robust, especially in the package manager setting. Doing a require with audit takes longer time, and increases the chance of different forms of timeout. In addition it opens up for the scenario where the composer require (or other commands) could exit with exit code 1, even if the install was succesful. In a programmatic way, this will for sure create confusion. Here is an example where I am faking an exception, to illustrate the state of it:
$ composer require psr/log
./composer.json has been updated
Running composer update psr/log
Loading composer repositories with package information
Updating dependencies
Lock file operations: 1 install, 0 updates, 0 removals
- Locking psr/log (3.0.2)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals
- Installing psr/log (3.0.2): Extracting archive
Generating autoload files
In Installer.php line 430:
some exception
require [--dev] [--dry-run] [--prefer-source] [--prefer-dist] [--prefer-install PREFER-INSTALL] [--fixed] [--no-suggest] [--no-progress] [--no-update] [--no-install] [--no-audit] [--audit-format AUDIT-FORMAT] [--update-no-dev] [-w|--update-with-dependencies] [-W|--update-with-all-dependencies] [--with-dependencies] [--with-all-dependencies] [--ignore-platform-req IGNORE-PLATFORM-REQ] [--ignore-platform-reqs] [--prefer-stable] [--prefer-lowest] [-m|--minimal-changes] [--sort-packages] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--apcu-autoloader-prefix APCU-AUTOLOADER-PREFIX] [--] [<packages>...]
$ echo $?
1
$ ls vendor
autoload.php composer psr
So I would argue using no audit is more robust, and more "right way". My personal opinion and 2 cents though :D
Still have not had time to produce any good results on test runs, but it seems the gain is small but consistent. Will try to post some results at some point :)
Does that mean you have tested and reviewed the code, and as such consider the addition reviewed and tested by the community? 🤓
Ah, makes much more sense. Thanks!
I'll produce some comparisons locally instead then ✌️
Hm this makes me a bit unsure if I am reading this correctly. Or even how consistent timings are across test runs or test runners. So I hope you will forgive me if I am completely misunderstanding and off track here... But...
https://git.drupalcode.org/issue/drupal-3491268/-/pipelines/357608 first full pipeline with fix. 7 mins 29 secs
https://git.drupalcode.org/issue/drupal-3491268/-/pipelines/357625 second run. Fix reverted for comparison. 9 mins 43 secs.
https://git.drupalcode.org/issue/drupal-3491268/-/pipelines/357678 third run, fix added back. 4 mins 45 secs.
Sounds kind of promising?
Well here is at least an attempt. I don't know the package manager codebase intimately, but this seems to be a common code path for the tests at least.
I also verified that the output before adding the flag included the string "No security vulnerability advisories found." (which comes from when composer is doing audits) and after I added it, it no longer was part of the output.
The result for me for running just one specific test was no actual gain in speed, but that was a relatively simple and fast kernel test. Let's rather see what the full run says
👌👍Opened 📌 Look into skipping audit of composer operations in package manager Active
Just randomly jumping in here, but from a quick glance it seems `update --lock' (and all other relevant commands) will run with audit enabled? We should probably also pass "--no-audit" or simply disable it altogether (especially for tests): https://getcomposer.org/doc/03-cli.md#composer-no-audit
I believe that could potentially also shave off some CI time
Forgive me if I missed it by glancing on my phone 🤓
Also additionally wanted to point out that it seems the assumption here is that user 1 does not have special powers. That is a setting in Drupal core, but it still defaults to the old behavior. User 1 is special unless you have specifically disabled it. Which would make more sense to check for first, instead of just assuming a role with that name exists and is even an admin role
This patch breaks my setup with 2 assumptions:
- That a role called administrator exist. It's pretty common of course, but far from required. In fact, you could have a role called administrator that only had access to "access content" should you want to.
- That any users with any roles even exist when trying to import content. I import content right after a reinstall in CI, at which point only user 1 exist
One solution to my problem would be to add back the old behavior as a fallback, making it at least a bit more backwards compatible. Would you be open to that?
As anecdotal evidence goes, I can verify this fixes the CI job that was failing without this applied.
The issue is that if we save a config file in the install hook, then that saving of the config will mess up for Drupal when trying to figure out which config to keep.
And we don't have to do that either, we can just update the config directly.
I also deleted the optional config, since it's not applicable, as gutenberg requires views.
eiriksm → created an issue.
Huh well I guess maybe the waiting might not be the problem after all? Anyway, leaving for review in case anyone has any input
>@eiriksm did you manually test it locally or no? That's our one gap in coverage. The tests are Node only and we need to write the Cypress tests for this still.
Hah, that would be the day.
I spent quite a while trying to get cypress to run this on a project. I found what I consider to be a bug, with a super easy to reproduce test case: https://github.com/cypress-io/cypress/issues/30405
I did not manually test it, no. I do however have a setup with my repo https://github.com/eiriksm/build-drupal-wasm which I am planning to get closer to what I consider "upstream" with drupal cms. The day only have so many hours :(
Anyway, the tests are pretty scarce but prove things are working for sure: https://github.com/eiriksm/build-drupal-wasm/blob/main/e2e/frontpage.spe...
Feel free to take stuff from there if someone has time
I should mention that I created this off of what I mentioned on the php-wasm discord. I have a CI system that does the bump for me basically, so we don't have to do this work manually and when we remember it
Haha, fair enough.
Updated
Hm there is quite a few failures but hard to imagine they are relevant to this MR
And what would be the actual consequences for projects that has already changed the sort order to case sensitive. Would it have to be reverted? I guess I could figure it out somehow on my own, but currently on my phone and just wondering.
Would be super nice to have some tests for this. Would you be able to fix that do you think?
Ah this is 1.x.
That's not supported any more
Huh must have mixed the projects in here
Thanks everyone ♥️🎉
Absolutely. If you use that module I don't think you need this one.
This one is if you have a custom module for ajaxifying the cart, and need to get rid of the message on the next page load
Also, this module is much much older than that. So very happy that a more complete solution exist 🤓
Yes because otherwise you would get the message "product added to cart" on the next page view, which is bad UX 🤓
Ah I see what you mean. That makes sense to me
I took a quick look, but then realized the diff was so long :o
I personally would like to merge this one first, since this way we would have tests in place for the image settings which are also touched there. Let me update the MR and you can give that a spin?
Great! That's also what's implemented in the (not yet merged) merge request. ✌️🎉
Could you test and/or review that, since it sounds like we agree on the way for an upgrade path?