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

Merge Requests

More

Recent comments

🇺🇸United States phenaproxima Massachusetts

This feature should be backported to 2.0.x.

🇺🇸United States phenaproxima Massachusetts

I have an idea on how to do this.

Project Browser, when applying a recipe, should check for a redirect directive in the recipe's extra section. If it's there, it's a YAML-encoded Url object (something we already support, for follow-up tasks). That's the URL you get redirected to. Simple.

But there's one restriction: if you apply multiple recipes at once, all redirects are ignored. You can only be redirected if you apply a single recipe.

🇺🇸United States phenaproxima Massachusetts

One question here about whether we're breaking backwards compatibility.

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

Gin released 5.0.4, so this is probably close-able.

🇺🇸United States phenaproxima Massachusetts

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

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Why was views.view.scheduler_scheduled_content.yml deleted?

🇺🇸United States phenaproxima Massachusetts

Reverted because this broke the site template under development in Drupal CMS. :(

🇺🇸United States phenaproxima Massachusetts

Reverted because this broke the site template under development in Drupal CMS. :(

🇺🇸United States phenaproxima Massachusetts

Shipped in 2.x with extreme prejudice.

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

@pameela, are you sure this is correct? AFAICT, this is adding alignment to one format, but not both. No objections to merging it, as long as the difference is legit.

🇺🇸United States phenaproxima Massachusetts

In the meantime we are also using simpleConfigUpdate and including the full set of matchers.

Not true anymore :) We are in fact polyfilling the action by synthesizing the full config action definition. It's a neat trick: https://git.drupalcode.org/project/drupal_cms/-/merge_requests/644/diffs...

But it would be better to have the real McCoy committed and released for us to rely on instead!

🇺🇸United States phenaproxima Massachusetts

Not a blocker for Drupal CMS, but why not also turn setMatcherConfig() into an action while we're at it? That could surely be useful too.

🇺🇸United States phenaproxima Massachusetts

We can't do this in drupal_cms_content_type_base, because we're introducing an implicit (and unnecessary) dependency on Canvas.

This should be done via a config action in drupal_cms_starter.

🇺🇸United States phenaproxima Massachusetts

Re-titled to remove DrupalCon mention, and merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

If there's a follow-up to be done, it's probably in Drupal CMS. We can mark this issue fixed.

🇺🇸United States phenaproxima Massachusetts

No objections to that fix. Needs a test though!

🇺🇸United States phenaproxima Massachusetts

Fair! I know what I personally would like the most, but I have no particular dog in this fight as long as it gets done somehow.

🇺🇸United States phenaproxima Massachusetts

To me, #3 is the most straightforward option. If we mess with the file cache, we're inviting ourselves to enter a world of pain. I also think the third option is the most "obvious" one, and therefore best for DX. I also like that it allows different plugin types to decide if they want to support additional properties or not.

I bet we could reduce implementation pain if we added a trait for plugin attributes that support additional properties.

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

The export is currently implemented by Drupal CMS Helper, not Canvas itself, and Drupal CMS helper can make this improvement.

🇺🇸United States phenaproxima Massachusetts

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

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

I've renamed the repository to https://github.com/phenaproxima/canvas-demo and updated Packagist to update automatically from there.

The old xb-demo URL redirects to canvas-demo -- thank you, GitHub!

I have not renamed the actual Composer package, but could get around to that if we really care about it. We'd need to keep xb-demo as an alias, I think, but Composer should support that with the provide or replace sections of composer.json, if I'm reading the relevant documentation correctly.

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

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

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Merged into 1.x.

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

After some discussion with @alexpott, I decided to revert this and do a quick hail-Mary patch of replacing all occurrences of experience_builder with canvas.

To provide a little more context: these components should never have been committed to the 1.x branch of Drupal CMS Olivero, but if I remember correctly, they landed before Drupal CMS 2.x was branched. So they kind of snuck in here, and are being used in certain corners of this theme (e.g., node--card.html.twig is using the card component).

So, realistically, removing them outright is not an option. But it is equally unacceptable for this theme to break Canvas.

There is probably more work to be done to make this theme play nicely with Canvas, even if it never fully embraces it. At the very least, we need a GitLab CI workflow which confirms that Canvas can be installed alongside this theme. I don't have the bandwidth for that right now, but I'll open a follow-up. Hopefully, the simple replacement I mentioned above will be enough to get past the immediate WSOD.

🇺🇸United States phenaproxima Massachusetts

I'm not sure on how it could have worked in the first place, if you were using components that reference schema objects from experience_builder -- which is now defunct?

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

@rodrigoaguilera, yes, a module could extend this by implementing hook_config_schema_alter() to change the options available to the validation constraint, then probably a targeted form alter to put additional options into the settings form.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Let's switch 2.x HEAD to Gin 5.x-dev, then!

🇺🇸United States phenaproxima Massachusetts

One question.

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

Merge train started to 2.x.

🇺🇸United States phenaproxima Massachusetts

After some discussion with @catch, I decided to add the link relationship as an additional configuration key, because trying to convert link_to_entity into a nullable string would put us into what can only be described as the 10th circle of update path hell.

But adding a new config key is straightforward, so I think this is ready for review.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

Backport MR opened; since this changes behavior of an existing API, should we maybe check with the release managers that this is backport-safe?

🇺🇸United States phenaproxima Massachusetts

Yeah, this is a duplicate. Closing tentatively for now. Be sure to update Drupal CMS Olivero to 1.2.4 or later before trying again, but feel free to reopen this if updating doesn't solve the problem for you.

🇺🇸United States phenaproxima Massachusetts

Merge train to 2.x started!

🇺🇸United States phenaproxima Massachusetts

I agree that'd be cleaner from an implementation standpoint, but I'd like @pameeela's sign-off on that.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

I was able to reproduce this problem, and I confirmed that it disappears with this bug fix. I'm gonna merge it and roll a new release, since this can interfere with people testing Canvas innocently on Drupal CMS 1.x.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

It's possible you're trying to use an older version of drupal_cms_starter that no longer matches what Project Browser expects.

Try updating drupal_cms_starter: composer update --with-all-dependencies drupal/drupal_cms_starter

If Composer complains that it's not installed, re-require it: composer require --update-with-all-dependencies drupal/drupal_cms_starter.

Does any of that help?

🇺🇸United States phenaproxima Massachusetts

This should be real easy. All we need to do is add this in drupal_cms_installer_theme_form_installer_site_template_form_alter():

$form['add_ons']['#required'] = TRUE;

Tagging as a novice issue.

🇺🇸United States phenaproxima Massachusetts

Yes! \Drupal\Core\Config\ConfigManagerInterface::getEntityTypeIdByName() will do what you want.

🇺🇸United States phenaproxima Massachusetts

Thanks for the bug report! I assume you were trying to spin up Drupal CMS 2.x to play with Canvas? (I don't know how this error could occur in Drupal CMS 1.x.)

Here's the thing -- Drupal CMS 2.x is still under very heavy development and is not yet stable or usable. Some of our dependencies are not tagged yet, and are changing on a daily, or even hourly, basis. These sorts of bugs happen all the time and we're fixing them as they come up. :)

So I'm gonna close this one out, I think, since it's not something we're liable to be able to reproduce reliably as our dependencies change. If you want to try Canvas, the best option right now is https://github.com/phenaproxima/xb-demo.

🇺🇸United States phenaproxima Massachusetts

We can override the select.html.twig template for this particular select list and style it however we want. This is probably not too tricky at all, honestly.

🇺🇸United States phenaproxima Massachusetts

@jurgenhaas showed me how to do this with ECA, but I think it would make more sense to do this when The config_save action should be able to treat its value as a partial config entity Active is available in a tagged ECA release.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Merge train to 2.x started!

🇺🇸United States phenaproxima Massachusetts

OK, I deleted every component that mentions XB in its metadata. I left the components which don't, since they're not harming anything.

Can you try this branch out to see if it still breaks in Canvas?

🇺🇸United States phenaproxima Massachusetts

Renaming to reflect the approach.

🇺🇸United States phenaproxima Massachusetts

The 2.x branch of this theme is unmaintained, and the 1.x branch is only minimally maintained. We don’t plan for this theme to ever fully support Canvas.

Having said that, it should not be causing a WSOD if you install Canvas.

So the solution here is probably to remove the components we added to it.

🇺🇸United States phenaproxima Massachusetts

Got it to work. It's a bit messy but it works. We can clean it up later.

🇺🇸United States phenaproxima Massachusetts

One nit but otherwise I have no complaints here.

🇺🇸United States phenaproxima Massachusetts

I could do form-element--site-template, would that work?

🇺🇸United States phenaproxima Massachusetts

Then let's remove it, rather than dealing with the rigmarole of making sure it doesn't show up Canvas.

🇺🇸United States phenaproxima Massachusetts

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

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

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

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x. Thanks for doing this!

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

We actually already have a template suggestion for those things specifically! I think it's input--radio--site-template, based on input--radio.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Reviewed, and looks better and better -- I think this will be even better if we use a generator function to yield the entities to export, rather than trying to load them all at once and do an array_chunk() on them. There is also more verbosity than is generally needed, and some changes which don't add anything to the patch and should be reverted. But it's shaping up!

🇺🇸United States phenaproxima Massachusetts

Damn you are fast. Assigning to myself to review.

🇺🇸United States phenaproxima Massachusetts

That makes sense. In any event, feel free to alter the markup if you need to, as long as the overall functionality remains intact. :) If this is ready, go ahead and kick it to needs review and I'll make sure @pameeela has a look at it.

🇺🇸United States phenaproxima Massachusetts

This seems like a great start -- and bless you for writing tests!!! -- but it's a little hard to read and understand, overall, because of the complexity. I think we might want to scale back a few things, at least for now:

  • The progress bar is a nice touch, but it adds weight to this MR and would be best done cleanly in a follow-up.
  • Let's not have the ability to just export all content, regardless of entity type. Such a function merits additional consideration and we probably would want to think it through in another issue. So, for now, let's require an entity type ID at the very least.
🇺🇸United States phenaproxima Massachusetts

For the record...if it will help style things as per the design, it is totally okay to convert that from a select list to something else. As long as changing the language redirects to the same URL with a different langcode query parameter, it doesn't matter whether it's a select or not.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Ah! Okay, that gives me the context I would need to fix it.

🇺🇸United States phenaproxima Massachusetts

We could remove it, but that would wipe out quite a bit of the value of the view for Drupal CMS (to the point where we'd probably need to ship our own override of the view anyway). We are already using the block display, because we put a list of recent pages on the admin dashboard.

🇺🇸United States phenaproxima Massachusetts

Explanation of current PHPUnit failures:

  • Drupal\Tests\canvas\Functional\ApiUsageControllersTest: Not sure what's happening here; a component was previously mentioned in an array and now it's...not? I don't know how adding a view could have caused this, but it feels like it might be exposing some other, preexisting bug, since the presence of a view can trigger many different code paths that were previously not exercised.
  • \Drupal\Tests\canvas_ai\Kernel\Plugin\AiFunctionCall\SetAIGeneratedComponentStructureTest: It passes locally for me, but was failing on CI. I have no idea how adding a view could have affected this either way. I decided to try to flail-fix it by having it install the canvas_page entity schema.
🇺🇸United States phenaproxima Massachusetts

Merge train started.

🇺🇸United States phenaproxima Massachusetts

Paired on this with @galactus86, so it has his sign-off. Merged into 1.x.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

How? There doesn't appear to be a route for creating a new Canvas page at all. The Page entity type doesn't define an add-form link template. Not seeing any such route in the routing.yml.

Am I missing something?

Setting back to NR for now, as the test failures appear to be unrelated.

🇺🇸United States phenaproxima Massachusetts

Okay, new idea here -- let's create two new ECA models:

  1. One which, whenever a content type is created, adds the SEO fields to it (and its form display). This will replace some of the config actions in the recipe.yml currently.
  2. One which, when the drupal_cms_seo_tools recipe is applied, executes that workflow (triggers the event, I suppose) for each extant content type.

Would that work?

🇺🇸United States phenaproxima Massachusetts

This issue is hard to finish.

We need to rethink the approach, because it's fundamentally incompatible with recipe unpacking. If you unpack the drupal_cms_seo_tools recipe (which you should), it is no longer visible to Composer -- and therefore, by extension, unavailable to ECA.

🇺🇸United States phenaproxima Massachusetts

That works as intended - merged into 2.x!

Production build 0.71.5 2024