🇺🇸United States @moshe weitzman

This would be hard for this module to get right. I suggest scheduling that entity_reference_revisions pruning drush command to run periodically.

I vote for a release ... We can deal with bug reports as they arrive.

Given that code review looks good, and automated tests are passing with this MR, I think we should merge and release. Or at least merge.

Made 1 fix to the MR. Now RTBC

moshe weitzman → made their first commit to this issue’s fork.

Any chance we can roll a D11 compatible release?

FWIW, I would not be surprised if automatic cron took some time to run, after it gets installed. In other words, I think it would be fine if it did nothing at install time. Its OK that module information would be unavailable for a bit.

mcdruid → credited moshe weitzman → .

was merged

composer ignores require-dev except for the root composer.json. So the effect of this addition is for this module's testing of itself and nothing more.

Devel maintainer here - thanks for doing this. I'm sure folks will like it. is there any way we could make the list of links in the new menu configurable? Thats what I was hoping for by building upon the Development menu. That way folks could use the menu UI to customize.

Merged a while ago

moshe weitzman → made their first commit to this issue’s fork.

That got merged into 11 and 10.4 so closing this. Please reopen if I am mistaken.

phpunit calls this PreConditions. Drupal module requirements have install time check as well.

4,x no longer maintained except major issues

Fixed a while ago

LGTM. Unfortunate that we need to wait for that dependency. Meanwhile, is there any testing we can add? Any upgrade path?

I wonder why tests are green even before this change.

moshe weitzman → created an issue.

Thanks. Once we have ✨ Directory based automatic service creation Needs review , we could add the RegisterListenersPass compiler pass and then get #[AsListener] classes registered as services and listeners. I'm not sure the intermediate step in this MR provides much value. If others think so, feel free to mark this RTBC.

Actually, I am wondering why we have both:

1. A service is declared in core.services.yml for maintenance_mode_subscriber
2. Various #[EventListener] appear on various methods on the same file. Wouldn't the nicer usage be to omit the yml declared service and rely on Attribute based creation of Listeners? The MR suggests that all we are saving is the getSubscribedEvents() method. I had higher hopes than that :). Maybe we need more plumbing elsewhere in core for what I describe.

This hasn't run since the switch to Gitlab 3 months ago. A bunch of D11 compat went in.

The ddev contrib plugin requires users to run `ddev poser` instead of composer install/update. that ensures a composer.json is present. See the readme for more info.

I reviewed the code and the draft CR. LGTM. Nice work.

📌 Replace ContainerAwareEventDispatcher with Symfony EventDispatcher Fixed was merged. Changing status per the most recent comments. Please update to a different status as needed.

That test approach sounds reasonable to me. Its OK that it may occasionally need updating if default batch size changes, and so on.

I dont see any library. I'd say we should truncate and keep the rightmost info in the url. The hostname is never useful.

Good idea. Followup created at https://www.drupal.org/project/dtt/issues/3477088 ✨ Implementing a PHPUnit\Event\Test\FinishedSubscriber that checks for conditions to fail the test Active . This MR is now merged.

moshe weitzman → created an issue.

Code is now removed.

Did you read my comment? We dont need to replace a use statement if its code is no longer needed.

Do we still need to call mutateBase? Maybe this code can be removed.

Sounds good. Maybe there is code out there that makes urls shorter in a smart way that saves useful information.

This got merged.

LGTM. Anyone object?

Thanks. Seems like there is nothing left to do in DTT. All our users will get this feature automatically when they upgrade to Drupal 11.1+. Before then, they can use https://gitlab.com/weitzman/logintrait

Drops Drupal 9 support as thats long EOL. This can be avoided if maintainers really prefer.

moshe weitzman → created an issue.

moshe weitzman → created an issue.

FYI, devel's tests started failing after this merge. Its not clear to me why, since devel uses lazy builders. https://gitlab.com/drupalspoons/devel/-/issues/541.

moshe weitzman → created an issue.

Superceded by https://www.drupal.org/project/entity_reference_revisions/issues/3474265 📌 Modernize Drush integration Needs review

moshe weitzman → created an issue.

Looks great.

Maybe add a line to composer.json to assure the commands keep working when folks upgrade votingapi.

"conflict": {
    "drush/drush": "<12.5.1",
  }

Agree with most of the proposed resolution. My thoughts:

1. Move long form texts to a new /docs folder in core (and contrib). The folder contains markdown files. Publish the docs via mkdocs or any other static site generator (SSG) that core prefers. mkdocs is the the SSG behind DDEVand Drush docs. We already support this in our Gitlab templates. Benefits:
   1. Once we are fully on Gitlab, it becomes super friendly to non devs and lazy devs to submit an MR via the Gitlab UI to change the docs.
   2. Docs can change along with code
   3. Docs automatically vary by branch.
2. Retire the api module and use Doctum or similar external project to generate our API docs. We can start with Drupal 12, and keep the API module docs as more or less static for older versions.

LGTM.

That issue is now fixed so this one is too.

$ composer require weitzman/drupal-test-traits --dev -vvv
...
Installs: weitzman/drupal-test-traits:2.3.1
  - Downloading weitzman/drupal-test-traits (2.3.1)
Downloading https://git.drupalcode.org/api/v4/projects/project%2Fdtt/repository/archive.zip?sha=5e3df23c93f4f3e4f34fd7677df10bc06616f4ce

I've come across this too when testing the Drush site:install command on sqlite.

IMO this is beyond the scope of this project. I suggest finding some code on Gitlab or elsewhere for this and adding a custom job.

moshe weitzman → created an issue.

moshe weitzman → created an issue.

moshe weitzman → created an issue.

moshe weitzman → created an issue.

moshe weitzman → created an issue.

Would be helpful if someone tried this MR with Drush recipe command - https://www.drush.org/13.x/commands/recipe/. Drush just includes the symfony command from Drupal as is so hopefully it works fine. Testing would be to make sure recipe options appear in command help and can be used successfully.

Released

I just created a release there for 2.3.1 and its been packaged. The Gitlab Packagist integration doesn't report any communication with packagist.org in its logs so this issue persists. I think we need to follow https://www.drupal.org/project/project_composer/issues/3464712 ✨ Regression: RC releases have empty dist information Needs work for updates.

Note that is affects DTT as well - https://git.drupalcode.org/project/dtt/. We just moved to drupal.org for project hosting and it would be nice to have dist downloads instead of git clone.

Would it make sense to try to serve these projects from the packages.drupal.org endpoint, and not rely on packagist? Or maybe some other way to avoid being recognized as a Gitlab repo type?

I've seen that too, for many years. Its frustrating. I did some experimenting and a possible fix for this is indeed to make releases in Gitlab, instead of just tags. That causes downloads of URLs like https://gitlab.com/api/v4/projects/weitzman%2Flogintrait/repository/arch... for the project https://gitlab.com/weitzman/logintrait. Unfortunately drupalcode.org prohibits making Gitlab releases. I get 404 at https://www.drupal.org/project/dtt/releases/new →

I found https://www.drupal.org/project/project_composer/issues/3464712 ✨ Regression: RC releases have empty dist information Needs work which looks very related.

I'm happy with the MR as is. Feel free to merge it when you are satisfied.

This was merged

hestenet → credited moshe weitzman → .

The other option is to remove drush as a dependency. Its implied. For same reason, we should remove the dependency on drupal/core IMO.

Was merged separately

IMO this is outdated at this point.

merged. thanks.

That sounds like a feature request for Pantheon (if their workarounds are not sufficient). Acquia and Platform support setting env variables, as do things like .htaccess and nginx config.

Like I said, DotEnv is really made for local development, so all this talk of web hosting demonstrates how people will be confused if this goes in. If people really want this feature, its a composer require away.

I could easily see us taking the base added here and doing a lot of special things in follow-ups. Like injecting the DB credentials for the primary/default DB automatically. Or providing an env variable override process for settings and config. But the base logic/support would be needed first.

I'm totally on board with settings.php honoring env variables. IMO thats the first part to do, not the last.

What I have seen is that .env files are helpful for local development, and less so for staging and prod servers which have other means of writing env variables. Given that we have standardized on DDEV for local dev, which already support setting env variables from config or .env files, do we really need this? The OP was written in an age before Docker based local dev.

It seems like the additional tests are not coming quickly. Would maintainers consider fixing the bug without tests? Otherwise we are forcing everyone to live with a bug because we are worried that the bug might return one day.

This module is no longer needed in 10.3+ https://www.drupal.org/project/drupal/issues/144538 🐛 User logout is vulnerable to CSRF Fixed

mass.gov was experiencing this issue as well. We worked around it by setting max-age=0 on the tabs block. See https://drupal.slack.com/archives/C1BMUQ9U6/p1715802644024269 for lots more discussion.

I mean, release branches wont get pipelines but thats not such a big deal. Projects that care can override the rules. IMO this can go in as is.

I think that one is OK because $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH will pass.

The Nightly scheduled pipeline at https://gitlab.com/drupalspoons/devel/-/pipeline_schedules is not running due to this.

moshe weitzman → created an issue.

This went in with https://git.drupalcode.org/project/domain_perm/-/merge_requests/2

moshe weitzman → created an issue.

We will be abandoning 1.x branch in favor of 2.x

Done in 2.x branch

See comment in MR.

moshe weitzman → made their first commit to this issue’s fork.

In my example I said "makes a new policy".

Your example is generally about the drawbacks of altering services from other modules. We have already settled that concern with permitted, but not recommended (see italicized warning) → . final takes this warning a lot further. Your example is a perfectly reasonable thing for a site to do, and the final makes it harder. Site developers don't need to care about multiple cooperating access policies.

Many cooperating policies is a rare use case. We've learned this with node access grants system - we built a system which handles multiple cooperating policies but few understand the system. We would benefit from a simpler and less cooperative system here IMO. Sorry I'm going off topic here.

IMO, this comes down to how paternal we want our code to be. I slightly prefer less paternal.

as soon as one module chooses to extend over decorating, the gate is immediately closed for all other modules to do the same.

If custom or contrib code makes a new policy and chooses to extend another (the original policy could be active or inactive - both valid use cases), why would all other active policies cease to work as designed? I must be missing something.

I dont feel too strongly about this. The OP makes some good arguments for this. Some counter-arguments:

• Its a bit unrealistic to have multiple access policies magically work together. Each site would have different requirements about what happens when the policies overlap
• Is decorating a class really much safer than extending it. You still "depend" on the inner class in a significant way.
• I'm from the "we're all consenting adults" philosophy. I would slap @internal on the policy classes and then any developer who chooses to extend does so at own risk.