Account created on 18 February 2015, about 10 years ago
#

Merge Requests

More

Recent comments

🇧🇪Belgium L_VanDamme

The seo_fields property is not a mapping, it is a list (potentially empty) of fields that need to be in the SEO wrapper.

https://git.drupalcode.org/project/rocketship_core/-/merge_requests/148/... needs to be changed

🇧🇪Belgium L_VanDamme

Using paragraphs is deprecated and will only receive security updates

🇧🇪Belgium L_VanDamme

Won’t do as this will be picked up as part of general layout builder improvements or deprecated in favor of experience builder

🇧🇪Belgium L_VanDamme

As mentioned in https://www.drupal.org/project/dropsolid_rocketship/issues/3438811#comme... Make the default page cache time higher then 1 day Active , we don't want to be patching core for this. We should simply put it on the highest value core allows or use a contrib module to increase it further.

🇧🇪Belgium L_VanDamme

We shouldn't patch core for minor performance upgrades. This introduces unnecessary update complexity.

🇧🇪Belgium L_VanDamme

Won’t do as the proposed modules will not actually fix the problem and feel like introducing unnecessary complications

🇧🇪Belgium L_VanDamme

Won’t do, we can use existing translation functionality (PO files) in stead

🇧🇪Belgium L_VanDamme

With the coming of experience builder, we won’t be adding functionality to layout builder anymore

🇧🇪Belgium L_VanDamme

Section library should be reworked as part of more general LB improvements or deprecated in favor of Experience builder. Closing for now.

🇧🇪Belgium L_VanDamme

Rerolled patch for 11.x (using my rebase commit of https://www.drupal.org/project/drupal/issues/2946333 📌 Allow synced Layout override Translations: translating labels and inline blocks Needs work MR 5956)

🇧🇪Belgium L_VanDamme

Rebased the MR from 11.x.

As far as I can tell, it still has issues when editing changed blocks after reverting revisions. I might take a look at that later, for now, it should work as before and apply to 11.x.

🇧🇪Belgium L_VanDamme

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

🇧🇪Belgium L_VanDamme

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

🇧🇪Belgium L_VanDamme

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

🇧🇪Belgium L_VanDamme

Looks good to me.

Small note: we might want to lose the default content (initial blog item) now that we have the add content by bundle button.

🇧🇪Belgium L_VanDamme

@phenaproxima I did another pass, properly rebasing, updating the licensing stuff to what is now in 0.x and moving the entity displays to config as we did for blog. Should be good to go now.

🇧🇪Belgium L_VanDamme

@phenaproxima I moved the entity displays back to the config folder (will do so for news as well in that ticket) and fixed the related blog posts view while I was at it.

🇧🇪Belgium L_VanDamme

@phenaproxima I changed the news description everywhere I could find it and added the licensing stuff. Having some troubles with a rebase though, can you have a look?

🇧🇪Belgium L_VanDamme

@phenaproxima As I understood it, we are planning to use layout builder on all content types. 1 thing that is in contention still is whether we'll be using the layout builder overrides for overview pages.

As for the view and form displays, I moved them to the recipe.yml as config actions to allow them to be overridden without causing issues when reapplying the recipe. This was due to a discussion about when we should allow for these kind of changes and when not, and the way I see it, if there's recipes in drupal_cms (or in a future version of it) that would alter the config, it might be better to have it as config actions and so allow overrides.
Let me know if this doesn't make sense.

🇧🇪Belgium L_VanDamme

@phenaproxima Updated the MR. I also took the opportunity to update the blog description and to add news to the CODEOWNERS file.

🇧🇪Belgium L_VanDamme

@phenaproxima Can you review? Build job is currently failing, but local test runs were ok for the news recipe.

🇧🇪Belgium L_VanDamme

@phenaproxima There's some tests failing but I don't think it is blog related. Let me know if there is something to fix.

Side note, I enabled layout builder and overrides for basic pages because we will need it to set up overviews that way. We can not use it at this point because core causes some issues when trying to import serialized layout builder sections (see https://www.drupal.org/project/drupal/issues/2942975 📌 [PP-1] Expose Layout Builder data to REST and JSON:API Postponed ). Depending on how much we want this way of working in v1, we might need to add some pressure there.

🇧🇪Belgium L_VanDamme

This patch is causing problems when reverting revisions.

This is because of the getActive call that was added to retrieve the latest version of the block when editing in stead of the saved revision. I believe it was added to prevent EntityChangedConstraint errors, but they were fixed in https://www.drupal.org/project/drupal/issues/3053881 .

Steps to reproduce this:

  1. Create a page with a text block with title AAAA.
  2. Update the page and change the text block title to BBBB.
  3. Revert back to the first version.
  4. The block title is now AAAA. But when you open the edit form of the block, it shows BBBB (the latest revision)

Code that is causing this behavior (in InlineBlock::blockForm()):

    if (!$this->isNew && !$block->isNew() && empty($this->configuration['block_serialized'])) {
      // Get the active block for editing purposes.
      $block = $this->entityRepository->getActive('block_content', $block->id());
    }

I think there might also be other places that this getActive function is called to prevent the error where it is no longer needed.

I would be willing to update the patch, but have not been active in creating it, so I'm not sure about what I might be breaking when removing this code.

🇧🇪Belgium L_VanDamme

Currently holding off on this until layout builder has been implemented in blog.

🇧🇪Belgium L_VanDamme

I had some discussions about this and the idempotency is still in tact without changing anything.

There are some changes that could be done like moving the entity displays to config actions, but really it will always be a balancing act, because it is hard to know what might get overridden an what won't. As a current decision, we will only move config to actions if we know for sure it might get overridden (e.g. when another recipe within Drupal CMS would change the config), which is not currently the case for any blog config. If the need arises in the future, separate tickets for the config items would seem to be the best approach.

🇧🇪Belgium L_VanDamme

My 2 cents:

I like most of what I read here, I think it's a very good place to start from.

I do agree with breidert on the configuration and interface translations being included. I don't know how much we would need to have configured by default, but they should be looked at IMO.

As for mandclu's comment about fields like "where" and "when" fields, I believe we should not make any field type translatable unless we're very sure what it does (e.g. only text and text_area and nothing else). IMO it is better to have a field not translatable that should be in stead of the other way around (because of the note in the edit form).

I agree with ignoring content moderation at this point.

I think metatag translations should be something for the SEO recipe. The most important ones will be generated based on fields anyway, and these fields (or their entities) will be translatable (title, description, image).

One other thing that might need to be discussed is the entity translation default settings (showing language select or not, interface language by default vs default language vs a language selected on recipe install).

🇧🇪Belgium L_VanDamme

+1 on using drupal_cms_anti_spam and article_comments as guideline.

Feel free to get started on this keeping those 2 things in mind, try to keep it simple to start to prevent too much refactoring later on.

🇧🇪Belgium L_VanDamme

@phenaproxima I removed the dependency on the content editor role. I guess there is not really a reason for a blog feature to be opinionated on user roles anyway. Also removed a leftover dependency in the drupal_cms content item.

Can you have a look again?

🇧🇪Belgium L_VanDamme

Some thoughts:

  • I agree with giving the cookies module a try. As mentioned, we're currently using EUCC and are not very happy with it. If anyone has experience with getting compliant implementations using the cookies module, do share any caveats or limitations you experienced
  • Including Cookie Content Blocker or a similar module will be required.
  • We currently use some extra modules to do things like adding datalayer events, extending the consent modal and providing ways to add consent based videos etc. I assume we would prefer to make a decision on which modules to use and then try to update these modules to include what we need rather than making an extension module?
  • We might want to do a little bit of research as to what is being used most for implementing tracking scripts etc. so we know what to support. E.g. Google Tag Manager, Matomo, ...
🇧🇪Belgium L_VanDamme

@jurgenhaas I went over the new division again and agree for the most part.

The only unclear item for me would be the "Subscriptions and unsubscription" being within the consent management recipe. I've not had much experience at all with implementing something like subscriptions in Drupal and it seems to me to be an edge-case that might need to be solved by the module supplying the subscription service in the first place. In that case, I would move this to the audit module, because that will make it possible for any subscription module to implement a plugin to have it checked.

Other than that, it looks good to me!

Production build 0.71.5 2024