Maybe run Composer with --no-scripts? Or maybe add COMPOSER_SKIP_SCRIPTS to your .ddev/config.yaml? (See https://getcomposer.org/doc/03-cli.md#composer-skip-scripts)
phenaproxima → created an issue.
I ended up having to de-flakify some tests because for some reason, the ordering of projects changes under certain circumstances. Why, I can't say, but I definitely confirmed two things:
- The tests I changed do not actually care about order; they were just relying on order-sensitive selectors because they didn't know any better.
- The EnabledSourceHandler is storing the same exact set of results, before and after this change, and returning them to the frontend. Svelte's rendering them in a different order, for some reason.
Given that, I decided that the correct course of action here is to improve the tests and make them more accurate, rather than trying to find out exactly why the ordering is different. If the ordering mattered to these particular tests, then the tests should, like, test that.
That seems out of scope - can we fix it in a separate issue?
Merged into 1.x. Thanks!
Adding 📌 Create more assertive wait methods for ProjectBrowserUiTestTrait Active and 📌 Add a more assertive waitForElementVisible() method to ProjectBrowserUiTestTrait Active as should-haves because they improve our test coverage in significant ways, and are already RTBC!
Started writing a test for this and then I realized that you actually cannot set the password on /user/register because the Password field is not even visible!
So the ECA model needs further tweaking for that.
Merged into 1.0.x. Thanks!
Merged into 1.x, thanks! I'm gonna err on the side of being conservative and not backport this to 1.0.x since the performance test job in that branch is allowed to fail (and is failing) anyway.
phenaproxima → made their first commit to this issue’s fork.
No objections.
Merged into 1.x and cherry-picked to 1.0.x. Thanks!
I think this is great. There are some things that could use tightening up and there might be a little missing test coverage, but this broadly seems like it covers all our bases. Amazing work.
Please do!
phenaproxima → made their first commit to this issue’s fork.
Merged into 1.x! Not backporting this one to 1.0.x, since it is definitely a feature addition.
This is a duplicate of 🐛 “Update” admin pages don’t belong under “Appearance” Active , and needs to be fixed in Automatic Updates. Closing as a duplicate, but transferring credit for that lovely and well-made issue summary. :)
phenaproxima → created an issue.
Awesome to get this done!! Merged into 1.x. Thanks!
This is definitely the province of Automatic Updates, so moving to that issue queue.
phenaproxima → created an issue.
phenaproxima → created an issue.
This probably needs to be rebased/adjusted after
✨
Feature: Include option in drupalorg_jsonapi source to show only locally installed modules
Active
. Even in 2.0.x HEAD right now, I no longer see any mention of a missing key in DrupalDotOrgJsonApi or its parent. So I'm not really clear on what's actually causing this. I'm guessing we need @fjgarlin's wisdom here.
Alright - that test coverage seems to be solid! Postponing on a new release of ECA
The Recipe command is provided by core, not Drupal CMS, and it is extremely weird for us to explicitly initialize install_state (to an empty array, no less, which might break all kinds of shit) in an install profile, where we can pretty much be guaranteed that it already exists, because install profiles only ever run in the installer.
I'm moving this to core, since this sounds like something that needs to be fixed there.
Ah, phew! That's a relief. Back to NW for me to finish the test coverage. :)
phenaproxima → created an issue.
This might have been a Drupal CMS blocker at one point but it no longer is, and certainly not for the Events recipe. Untagging!
This is no longer a Drupal CMS dependency critical; it's merely of interest. "Dependency critical" means we'd have to boot the dependency out of Drupal CMS if this weren't fixed in time, but XB currently isn't a dependency.
I would vote we remove it for now and fix it in a follow-up.
How to test, now that 📌 Add Experience Builder to dev dependencies Active is in:
- Check out this branch locally
- Pull changes from 1.x and merge them into this branch
-
ddev rebuild ddev drush si -y ddev drush recipe ../xb_test ddev launch /xb/xb_page/1
How to test, now that 📌 Add Experience Builder to dev dependencies Active is in:
- Check out this branch locally
- Pull changes from 1.x and merge them into this branch
-
ddev rebuild ddev drush si -y ddev drush recipe ../xb_test ddev launch /xb/xb_page/1
Merged into 1.x. Hope this helps contributors!
phenaproxima → created an issue.
Looks like this has merge conflicts, can those be resolved before I merge this?
phenaproxima → created an issue.
Started writing a test and there seems to be a bug here -- there are Simple Sitemap configurations being created for node types that don't even exist, like index and yoast_seo: https://git.drupalcode.org/project/drupal_cms/-/jobs/4414792#L42
Possible bug in the ECA model?
Merged into 1.x.
phenaproxima → created an issue.
Merged !7 into 1.x.
Gave this a quick manual test in the context of Drupal CMS and confirmed that it works as expected (the state key contained the names of the recipes I selected at the beginning of the process).
Manually tested this in the context of Drupal CMS and confirmed that it does indeed skip the RecipesForm step if there is no recipes[optional] key in the profile's info file.
phenaproxima → created an issue.
Postponing on 📌 Move the Project browser specific styling from gin theme to project browser's scope. Active .
I think that looks pretty good, just a few small changes.
Finally merged into 1.x, thanks!
Alright, folks seem to agree this looks good and I can find nothing to complain about in the code. Sick of seeing this one languish in review forever, so I'm merging it.
This seems like a great start and is straightforward. I think we might want to add some test coverage for the keyboard functionality, though. There might be preexisting test coverage in core you could use?
First should-have has landed! ✨ Feature: Include option in drupalorg_jsonapi source to show only locally installed modules Active
Fully agreed that we need this. I can get this written pretty fast.
Ooh, good find. Yeah - it wasn't properly populating the non-volatile project store because the results page was still returned under the name of the decorated plugin.
That's fixed now and there's test coverage to prove it. I also gave it some local manual testing and it worked as intended.
OK, well, I'm tentatively RTBCing this since the code itself looks fine, but I would like it to be confirmed in manual testing by someone familiar with the look and feel that is desired here.
If that can't happen, for some reason, then a before/after screenshot pair would be ideal.
This seems like great clean-up and it will help us actually catch bugs. I didn't review the .gitlab-ci.yml changes in depth, to be honest, but I'm not particularly concerned about those anyway. The pipeline looks like it's running, and passing, and that seems right to me. I don't really feel like blocking quality improvements. :)
Opened ✨ EnabledSourceHandler's query result caching should also consider the contents of composer.lock Active to address #26 separately. I'm guessing it should also be an alpha10 should-have.
phenaproxima → created an issue.
Merged into 1.x and cherry-picked to 1.0.x, as this is a test-only change. Thanks!
Made a few changes:
- Everything now happens in checkSitemap(), so that the test happens for authenticated and anonymous users
- I don't bother testing the admin side, as it's beside the point. We care that the sitemap looks a certain way, not that configuration flags are correct
I think this test is worthwhile to have but I'm not sure it's testing the most useful aspects of this.
For example, we don't need to check that the specific plugin checkboxes are checked, we just need to be sure that /sitemap has what we expect it to. Ideally we would not rely on CSS selectors at all, since they can change out from under us.
Self-assigning to see if I can optimize this a bit.
Merged into 1.x and cherry-picked to 1.0.x. Thanks!
Whoops, guess I accidentally committed this to 1.0.x first. Maybe that's what the MR targeted. Which is wrong, but not the end of the world. Perhaps in GitLab I can prevent MRs from being opened directly against release branches.
I don't see any harm in backporting this to 1.0.x but it does mean we'll want to update the constraints for drupal_cms_ai in that branch.
Replying to #17 -- this was recommended by @tim.plunkett as a good future proofing step for when (it's probably not a question of "if") we want to add more plugins that query drupal.org in opinionated ways. We were able to do a decorator this time, but that might not always be the case. I personally don't see too much harm in splitting the d.o API parts into a base class but we could revert that if you feel really strongly about it.
I thought about this a bit and I realized we probably don't even need to read composer.json if we stop using the event subscriber and just add the "mark as applied" stuff as a normal batch operation. Opened !7 to propose this. What do you think - will that work? I like it most of all because it keeps everything in the Hooks class and minimizes the amount of code that a generated profile will have.