Massachusetts
Account created on 15 November 2007, over 17 years ago
  • Senior Software Engineer at Acquia 
#

Merge Requests

More

Recent comments

🇺🇸United States phenaproxima Massachusetts

#4: Those errors seem to be unrelated to this issue. But, to give you some detail:

Unsupported Composer plugins were detected.
civicrm/civicrm-asset-plugin
civicrm/composer-compile-plugin
civicrm/composer-downloads-plugin

For safety's sake (for the time being, anyway), Package Manager is very strict about which Composer plugins you're allowed to have in your site. To get past these, you want to add them to the additional_supported_composer_plugins config setting in package_manager.settings. You can do that in settings.php or with Drush. (There's no UI for this.)

Problems detected related to the Composer plugin cweagans/composer-patches. It must be a root dependency.
The composer-exit-on-patch-failure key is not set to true in the extra section of composer.json.

Package Manager has historically supported this plugin, but with caveats. You have to have the plugin listed in your site's composer.json explicitly -- it can't be brought in by some other dependency. And you need the specific exit-on-patch-failure configuration option set for it. Having said that, it would be best to remove the plugin entirely, if you can, because it's about to become disallowed by default in 📌 Package manager/ Automatic Updates should disallow composer patches by default Active .

🇺🇸United States phenaproxima Massachusetts

I think we need this. Right now, the documentation (in Package Manager's hook_help) tells you to use Drush, but this is not always an option for site builders, especially not on cheaper hosting.

Since the executables' locations is already configurable, this would not hurt the status quo.

🇺🇸United States phenaproxima Massachusetts

Seeing as how ComposerPatchesValidatorTest failed until I specifically enabled the setting in that test, I'd hazard to guess that there's probably no additional test coverage needed here. Crediting myself for the MR.

🇺🇸United States phenaproxima Massachusetts

/../active_directory/composer.json

/../active_directory/web/tmp

This is probably the core of the problem. The temp directory needs to NOT be inside the active directory. In other words, something like this would work better, if you can configure your set-up accordingly:

../active_directory/composer.json

/tmp

🇺🇸United States phenaproxima Massachusetts

Adjusting credit, since the MR is a slimmed-down version of the patch in #7.

🇺🇸United States phenaproxima Massachusetts

In Package Manager terminology, the "active directory" is the root of the Drupal project: basically, wherever the main project-level composer.json is.

The "stage directory" (which we are going to rename "sandbox directory" for clarity) needs to be in a temporary directory that is outside the Drupal project. Usually Package Manager will try to compute it by asking the system where the overall temporary directory is (for example, /tmp on most Linux systems), so if the temporary directory is, for some reason, a subdirectory of the Drupal project, you'd get this error.

🇺🇸United States phenaproxima Massachusetts

This issue definitely belongs in Drupal core, since that's where Package Manager comes from.

🇺🇸United States phenaproxima Massachusetts

I'm generally +1 for this, because I like anything that streamlines the installer and gets people up and running more quickly.

🇺🇸United States phenaproxima Massachusetts

Discussed in Slack and I am convinced that, although the more aggressive cache flush is a ding for performance, that concern is outweighed by the utterly baffling shittiness of a cache that has been accidentally primed with garbage during the install process, rendering Drupal non-functional unless you are able to drush cr.

🇺🇸United States phenaproxima Massachusetts

Cool. This sounds like it's more of a frontend issue, then, and not really a data integrity problem (and thus, I'd imagine, not a stable blocker).

🇺🇸United States phenaproxima Massachusetts

I have a question that either Wim or Lauri need to answer, brought up by this issue.

The way the MR currently works, a content template that contains a dynamic prop expression targeting a field -- base field override or normal bundle field -- will cause the template to be dependent on that field.

That makes sense from a data integrity standpoint, but are we not setting up a situation where, if a site builder deletes a field, all templates that mention that field are deleted too? That seems extremely bad.

What should we do here? We probably need ContentTemplate to implement onDependencyRemoval() to handle this case in some way, or XB to be able to show a graceful fallback (assuming it doesn't already) if a prop expression is referring to a field that no longer exists. Either way, this needs product and tech lead input, so assigning this to Wim before we do any more work on it.

🇺🇸United States phenaproxima Massachusetts

I did some work on this to move the dependency resolution into ComponentInputs. I also made sure that it checks for base field overrides, which are another kind of config dependency (and we do need to test for this).

🇺🇸United States phenaproxima Massachusetts

This will also need a change record, since fixing the bug on existing sites will require manual steps.

🇺🇸United States phenaproxima Massachusetts

In addition to a new issue title and summary, it would be nice to have test coverage in seo_tools to confirm that our meta tags result in a HIT for the x-drupal-dynamic-cache header. Since the recipe already has functional test coverage, this shouldn't be too hard to do.

🇺🇸United States phenaproxima Massachusetts

Okay, this has a test now, so removing the tag. There are still some data integrity questions here, though, which surely means this is a stable blocker.

🇺🇸United States phenaproxima Massachusetts

Okay -- needs test coverage (quite a bit of it, I'd think), and validation, but I've at least implemented my first stab at the render-time logic. Could use a quick review to confirm I'm not way off-base here.

🇺🇸United States phenaproxima Massachusetts

While discussing this with @catch in Slack, I realized that RequireEventTrait could be better-named. There is nothing about it that specifically has anything to do with a composer require operation -- if, say, we implemented a composer remove capability in the future, and wanted to dispatch a pre- and post-event for that, then we'd want to also use something like RequireEventTrait, but it would be confusingly named for that scenario.

I've renamed it to the clearer and more accurate EventWithPackageListTrait. This could have been done in its own issue, but while we're getting everything ready to go with better permanent names in this one beta-blocking issue, I figured now was the easiest time to do it.

🇺🇸United States phenaproxima Massachusetts

This is a support issue until it's proven that it's an actual bug in Automatic Updates or Package Manager.

🇺🇸United States phenaproxima Massachusetts

Well, this is a tricky one. If it's the same issue about composer validate failing when Package Manager runs it, then it's honestly hard to say without getting the full error.

You could try hacking Package Manager to write the full output of the error to a file, which you could then attach to this issue. It's maddening that it doesn't seem to happen if you run the same damn command at the command line, though!

🇺🇸United States phenaproxima Massachusetts

Been contemplating this for a couple of days.

I think what each item in a multi-cardinality XB field needs to look like is...a full tree. With a root UUID and everything.

The reason I need a root UUID is so that the content template, which assembles the final renderable tree, knows which part of the tree goes directly into the exposed slot, and which parts of the tree are just NestedArray::mergeDeep()'ed into the main tree (since it seems that the tree structure is never more than three levels deep).

In other words, given a single mini-tree loaded from an entity and targeting a particular slot in the template, the root of the mini-tree goes directly into the exposed slot, and the rest of the tree is just added to the main tree.

🇺🇸United States phenaproxima Massachusetts

Re-titling for clarity. @catch endorsed the two-issues idea in Slack.

🇺🇸United States phenaproxima Massachusetts

I think this is ready for an initial review. It doesn't change any commentary, only runtime code.

  • StageBase is now SandboxManagerBase.
  • Events that have a $stage property now have a $sandboxManager property.
  • getStageDirectory() and stageDirectoryExists() are now getSandboxDirectory() and sandboxDirectoryExists().
  • Any parameters or local variables named $stage are now $sandbox_manager.
  • Exceptions that began with "Stage" now start with "Sandbox" (e.g. StageException is now SandboxException).
  • The idea of "operations" vs. "phases" is still fuzzier than I'd like -- I think that create, apply, and destroy should be considered "phases", and anything that happens between create and apply should be considered an "operation". But having said that, PreOperationStageEvent was named years ago, before we understood that the "pre" events are central to validation. But since that is their superpower (the ability to log errors), I have renamed that to SandboxValidationEvent to clarify its purpose.
  • A couple of tests and test methods were renamed, but generally the tests were not altered.
🇺🇸United States phenaproxima Massachusetts

This is a huge scope of work. I'd like to propose that we split up the work here into two issues.

  • In this one, which should block beta, we just rename the classes as appropriate, but let "stage" persist in comments.
  • In another one, which can be stable blocking, we change all comments, but no runtime code whatsoever, to use the updated terminology.
🇺🇸United States phenaproxima Massachusetts

A few proposals:

  • StageBase should become SandboxManagerBase, to more accurately describe what it does. Hopefully, this will completely disambiguate the concept of an object that manages the existence and lifecycle of a sandbox. Generically these will be referred to as "sandbox managers"
  • "Sandbox" should refer to the temporary copy of the Drupal project, as a whole. This is more common term than "stage", but refers to the same concept, and is less likely to be confused with a verb.
  • "Sandbox directory" specifically refers to the directory where the sandbox is located.
  • "Create", "require", "apply", "post-apply", and "destroy" can be consistently referred to as "phases", e.g. "the create phase", "the apply phase", etc.
🇺🇸United States phenaproxima Massachusetts

I should note that the solution in #83, while great, doesn't actually remove all risk to site operators. Consider:

If you do require --dev recipe we should not auto unpack and tell people that dev recipes are not unpacked

This will still bring in modules and themes, as indirect dependencies of the recipe. Therefore, you could still break your site with composer install --no-dev.

But, @alexpott and I feel that this is an acceptable trade-off, for a few reasons:

  • This isn't a whole lot different than directly adding modules and themes to require-dev. It's just that the recipe adds a layer of indirection. So it's a tad worse, but not much.
  • For the recipe to be a dev dependency, you would have had to specifically run composer require RECIPE --dev. It would have been an explicit choice by a developer.
  • If you add a recipe to a site via Project Browser, it won't add it as a dev dependency. This is what we would expect most site builders to do (in Drupal CMS, anyway).
  • But even if a site builder did add a recipe at the command line, composer require's default behavior is to add the recipe to require, not require-dev.

If, despite these mitigating factors, you do somehow manage to get a recipe into require-dev, the unpack plugin will try to nudge you in the right direction, as #83 states:

If you call unpack on a dev required recipe we tell you we’re going to move it to require and unpack

And ask the user if that is okay

If no don’t unpack [i.e., keep the dependencies in place]

If, after all this, you still have a recipe in require-dev which has introduced runtime dependencies, and you have explicitly refused to unpack it into require, then that's on you, o mighty power user. :)

🇺🇸United States phenaproxima Massachusetts

@alexpott and I discussed this extensively in a Slack DM.

Here was the long thought process I had, for those interested:

My current thoughts on this:

What should happen when recipeA is in require-dev and recipeB is in require and you call unpack recipeA?
Everything goes to require because some of recipeA’s dependencies are in require, although a more fiddly way to do this would be to “switch modes” midstream such that everything at or below recipeB in the dependency tree, is unpacked to require. But that feels more complicated than we need to bother with.

What should happen when recipeA is in require-dev and recipeB is in require and you call unpack recipeB?

Everything goes to require. You are explicitly unpacking something in require, therefore everything under it goes to require.

What should happen when recipeA is in require and recipeB is in require-dev and you call unpack recipeA?

Everything goes to require. For the same reason as above. recipeB is moved to require because it’s a dependency of recipeA.

What should happen when recipeA is in require and recipeB is in require-dev and you call unpack recipeB?

Everything goes to require. recipeA depends on recipeB, and recipeA is a requirement, which implies that all of its dependencies are too. (edited) 

Another way to put my thinking on this is: require always has more “gravity” than require-dev (edited) 

For the simple reason that, semantically speaking, require means “I cannot function without this”

It is also worth mentioning that we don’t need the unpacking to be “perfect”. If it moves something into require that’s really more dev-oriented, we actually have no way to know that, because we don’t know how the site builder is using that module or theme in their specific context. It can and should be up to the site builder to sort that out. Our priority needs to be not breaking sites (edited) 

^^ Which, now I think about it, is actually a pretty compelling reason to always unpack to require, unconditionally, and never do require-dev.

To put it another way: some module/theme that originally came in as a dev dependency might actually end up being used, in practice, as a production dependency. And Composer would have no way to know that. We’d have to rely on the site builder knowing how Composer works, which we absolutely cannot rely on. (edited) 

Here’s another reason to avoid require-dev: it can introduce confusion. Someone might require a recipe into require-dev with the idea that “well, I’m only using this recipe to help build the site, and then I’ll remove it” — that’s a reasonable line of thought. But then let’s say they do exactly that, and now they have their working site, but have unwittingly unpacked all these runtime dependencies introduced by the recipe into require-dev. They are in a prime position to WSOD if they unwittingly do composer install --no-dev , which is best practice in production. (edited) 

So I dunno. require-dev for recipes at all might be really bad news.

It might have made more sense back when there was this idea that the unpack plugin could handle any kind of package. But for recipes specifically, now that we’ve narrow-scoped it? Not sure.

And here's my summation, your honor:

Recipes introduce dependencies on modules and themes that Drupal will expect to be present, or it will WSOD. If you have any of those dependencies in require-dev, you are one composer install --no-dev away from breaking your site completely. (Omitting dev dependencies from production is a best practice.)

We cannot safely assume that site builders, who may have zero understanding of or experience with Composer, will know that installed modules and themes effectively must to be in composer.json's require section.

It is also much less important for unpacking to result in "everything in its right place", than for it to result in a working site that isn't broken.

@alexpott proposed what I think is an excellent solution that sidesteps the problems outlined in #82:

I think we should only support unpacking recipes in require

If you do require --dev recipe we should not auto unpack and tell people that dev recipes are not unpacked

If you call unpack on a dev required recipe we tell you we’re going to move it to require and unpack

And ask the user if that is okay

If no don’t unpack

I think with those rules we’ll have way less complexity [and magic]

Yes. YES! A thousand times yes to this.

🇺🇸United States phenaproxima Massachusetts

I still am -1 on implementing this workaround because it's almost guaranteed to turn into long-standing technical debt if the underlying issue doesn't get fixed in a timely fashion.

A little more research would be ideal, I think -- what would it take to fix this upstream, or at least mitigate it? If the fix is complicated, then okay, what you propose here would be okay as a temporary workaround. But if it's relatively simple -- especially since it's in a contrib module -- we might just be better off getting it fixed there.

🇺🇸United States phenaproxima Massachusetts

I don't love this, because we are definitely not going to remember to update these in 2026.

To me, a better solution -- and probably not something we can fix in Drupal CMS specifically -- is that the tokens should influence the cache lifetime. It shouldn't make the page uncacheable. It's just that, if you use, say, the [date:html_year] token, then the maximum cache lifetime is reduced to a day.

What module defines the [date:html_year] token? If it's core, maybe this is a core bug.

🇺🇸United States phenaproxima Massachusetts

Two threads are still unresolved, but they need answers from Wim (or someone else knowledgeable). Otherwise this is probably ready, as far as I can tell.

🇺🇸United States phenaproxima Massachusetts

But Editor is a config entity too, so that does seem fine?

Editor uses this pattern for a subset of its values (image_upload.1 and image_upload.0), but the config schema type for the whole entity is not polymorphic.

🇺🇸United States phenaproxima Massachusetts

Pretty simple - I think we just want to mention that \Drupal\layout_builder\SectionListTrait::setSection() is now a public method and will be available on every class that uses SectionListTrait.

🇺🇸United States phenaproxima Massachusetts

Agreed -- I have added a comment that should hopefully make clearer how the render execution flow works. Thanks for pointing that out.

🇺🇸United States phenaproxima Massachusetts

Looks great. I have basically no complaints about the action itself. My only outstanding issues:

  • Needs two change records before I can RTBC: one to document the action itself, the other to mention the publicizing of setSection.
  • The test is a bit repetitive and could be refactored for clarity by using a data provider.

I had a couple of minor nitpicks as well. But this doesn't feel far off to me.

🇺🇸United States phenaproxima Massachusetts

This actually needs to be in Automatic Updates, which is where that text originates.

🇺🇸United States phenaproxima Massachusetts

So I cannot use config schema by itself to enforce that exposed slots are only allowed in the full view mode, for a couple of reasons.

  • This is a config entity, which means there's always an exposed_slots property, even if unused. If it's present (which it will be) in content templates in other view modes, those are automatically config schema errors.
  • experience_builder.content_template.*.*.full cannot resolve to a type unless it was also based on some kind of meta-type like experience_builder.content_template.*.*.[view_mode]. There's precedent in core for something like this (editor.schema.yml), but not for config entities.

The proper path here looks like a validation constraint at the entity level which confirms that exposed_slots is empty in a non-canonical view mode. I think it probably makes more sense to just graft this into the ValidExposedSlot constraint.

🇺🇸United States phenaproxima Massachusetts

Wim and I talked about this issue today and we agreed to split it into two issues:

  1. This issue, which will add the exposed slots data structure on the ContentTemplate entity itself, with validation. It won't be used for anything yet.
  2. In another issue, the entity-side storage for what goes into the exposed slots. Wim feels that this should involve making the component_tree field item support infinite cardinality, and adding a nullable slot_name column to it. This is appropriate for a number of future-focused reasons that Wim articulated to me, along with making it much easier to detect if a content creator is about to make a destructive change that will affect exposed slots in extant entities.

The only work still needed here, I think, is making sure that only the content templates for the full view mode actually allow exposed slots. Hopefully this can be done with some config schema magic, but it might call for a new validation constraint.

🇺🇸United States phenaproxima Massachusetts

Docs updated. I believe all feedback is now addressed, although there is one commented-out assertion that I think I need @larowlan's input on.

🇺🇸United States phenaproxima Massachusetts

To be clear on this, the launcher vs. DDEV is not an apples-to-apples choice. The launcher is not really a proper development environment, since there's no easy way to port the database from one environment to another (at least, not without some hacking). Maybe at some point the launcher will be a reasonable way to start a site, but for now it's really best suited for trialing Drupal CMS.

🇺🇸United States phenaproxima Massachusetts

I think this is almost certainly an issue specific to your installation of Drupal. Having said that, with a truncated error message to work, I unfortuately can't pinpoint the problem.

If you're able to SSH into your server, can you try running the following at the command line, and paste the full output here?

/usr/bin/composer validate --check-lock --no-check-publish --with-dependencies --no-ansi --working-dir=/var/www/cms

(tl;dr: This is may be some syntax issue with composer.json or composer.lock, if it's the same error that you mentioned in the issue summary)

🇺🇸United States phenaproxima Massachusetts

This is quasi-blocked on Content templates, part 2: Using the templates for rendering Active , in that I'd rather build on the test coverage being added over there than have to scaffold in wholly new test coverage.

But, everything else about that can happen now, so I'll start on that tomorrow.

🇺🇸United States phenaproxima Massachusetts

Updated Content templates, the boss battle: create a UI for editing templates Active with what was discussed in #20 as far as opt-in goes. So we don't need a separate follow-up here.

🇺🇸United States phenaproxima Massachusetts

We also agreed that opting content types into XB properly (i.e., ensuring the proper view modes exist, and the XB field) is not in scope here. We'll have to open a follow-up for it. What this issue and test coverage does confirm is that the presence of a component_tree field is the indicator that a content type is opted in.

🇺🇸United States phenaproxima Massachusetts

Package Manager originated in Automatic Updates, which is where the discrepancy is coming from. The 4.x branch of Automatic Updates doesn't contain Package Manager, and relies on core's version.

🇺🇸United States phenaproxima Massachusetts

Discussed this with Lauri and Callum on Zoom. Here's the plan:

Node types are explicitly opted into XB (let's assume it's with a third-party setting). If you don't opt into XB, the node type is handled exactly the way it currently is. Nothing changes for site builders, content creators, or site visitors. You can opt a node type into XB, and later opt out; neither choice causes any data loss.

If you do opt a node type into XB, the site builder experience and site visitor experience are both changed. Let's examine each path separately.

For site builders:

  • Visiting the node type's "Manage display" page will always kick you into XB.
  • If there is no existing template for the canonical view mode (which we can currently assume is either full or default), you see an error-like page that prompts you create a template for that view mode. If there is an extant template for the canonical view mode, that'll be the one you're editing by default.
  • The other enabled view modes (e.g. RSS, Search index, etc.) are visible in the sidebar, under "Templates". If you click on any of those, you being creating (or editing) a content template for that view mode.

So, to summarize: when you opt a node type into XB, you manage the displays for that node type in XB only.

For site visitors:

  • When rendering an entity, we look for an XB template in the requested view mode. It status doesn't matter; its existence is all we care about.
  • If we find one, great! Render it.
  • If we don't, then fall back to core's rendering.

To summarize: we render with an XB template if one exists for the specific view mode you asked for. But we fall back to core rendering at the drop of a hat, so as not to break existing sites, and to make life easier for new sites.

🇺🇸United States phenaproxima Massachusetts

I don't mind that, but what if templates exist for every view mode except full? (An edge case, to be sure, but not an unrealistic one, especially since most people likely just configure default, not realizing that it is being used for canonical rendering.)

🇺🇸United States phenaproxima Massachusetts

@jose reyero, the translating is a bit tricky because a lot of the new, untranslated strings come from the installer's custom theme, which is, well, a custom theme and not a standalone project.

Does it need to be moved into its own project for this to work?

🇺🇸United States phenaproxima Massachusetts

That is, indeed, what I was imagining.

I'd probably start with some relatively low-hanging fruit, like, say, if the pageSize store value changes, it should automatically trigger a data reload (the load() function in ProjectBrowser.svelte).

🇺🇸United States phenaproxima Massachusetts

Looking at the designs, a thought: it seems that, with XB enabled, if you go to "Manage display" in the Field UI, you'll be kicked into XB. So what if we did the following:

  • Require a node type to opt into XB, maybe as a third-party setting on the node type form.
  • Once that's done, "Manage display" will send you to XB, and you can edit/create a template in the given view mode.
  • When rendering, XB will try to either find a template for the requested view mode, or it falls back to core so as to not break existing sites.

This doesn't feel super elegant but maybe it's the most pragmatic solution that remains true to the designs, while not breaking existing sites.

🇺🇸United States phenaproxima Massachusetts

I'm glad I split this into a separate issue, because after some discussion with @mglaman, this definitely opens up a (hopefully relatively small) can of worms.

Core doesn't really have the formal concept of a canonical view mode. It has hard-coding that treats the full view mode -- supplied with all core content entity types -- as "canonical" in the sense that it's the view mode you see if you look at the entity on its own page. It is possible for that view mode to be something other than full, though.

Complicating this is the fact that, due to the way the Field UI works, people probably configure the fields and whatnot in the default view display, and core falls back to that in most cases because the full view display probably doesn't exist...until you explicitly enable it.

And, to pile on even further, when you opt a bundle (I'm just gonna say node type from here on out, for expediency) into XB, what happens to existing view displays? Does a template get created for the full view mode only? Or do templates get created for all view modes? If it's the former case, then what happens when you view an entity in teaser but no XB template exists for it? If it's the latter case, how do you set up the teaser template initially? We almost have to fall back to core rendering, so as not to break sites that enable XB with content and view displays already set up.

We need to think through these things a bit more before implementation can begin in this issue. But none of this need block Content type templates, part 1: introduce a ContentTemplate config entity which always renders an empty component tree for a particular entity display Active .

🇺🇸United States phenaproxima Massachusetts

I think I've fixed all the feedback so far.

After some discussion with @lauriii, the scope here can be reduced further. Wim pointed out that the "rendering algorithm" came sorta out of nowhere, so I asked @lauriii about it and he came up with the rendering flow he would actually like XB to implement. It's not in scope here, so I'm punting that to a separate issue that I have yet to open.

So indeed, this issue is just about adding the ContentTemplate config entity (the class name was blessed by @lauriii), but not actually using it anywhere. That's a reasonable first step.

🇺🇸United States phenaproxima Massachusetts

OK, this is pretty much where I want it. After discussion with Lee and Matt, I don't think this needs anything more than kernel test coverage. We don't need to test actual rendering yet, since there's nowhere for the template to store the component tree (yet). We just need to ensure that the correct template is chosen in specific situations.

🇺🇸United States phenaproxima Massachusetts

phenaproxima made their first commit to this issue’s fork.

🇺🇸United States phenaproxima Massachusetts

I agree that tagging particular YAML strings as translatable is follow-up material.

Production build 0.71.5 2024