Ljubljana
Account created on 25 August 2017, almost 8 years ago
  • tech journalist && occasionally helps w/ webdev at Radio Študent 
  • Backend Developer at drunomics 
  • Community organizer, Web developer at Kompot 
#

Merge Requests

More

Recent comments

🇸🇮Slovenia useernamee Ljubljana

Code looks good and provides a relevant Kernel test. RTBC

🇸🇮Slovenia useernamee Ljubljana

Kernel test is very detailed and I think it thoroughly covers the XB to CE conversion.

🇸🇮Slovenia useernamee Ljubljana

I checked MR9 with upgrade status and the scan did not return any known issues. Moving to RTBC.

🇸🇮Slovenia useernamee Ljubljana

There seems to be little to no activity here. We (me and @fago) are still eager to co-maintain this module and add the improvements from the ticket description in the 3.x version.

🇸🇮Slovenia useernamee Ljubljana

passed-tr, I also tested it locally, works, pipeline is passing

🇸🇮Slovenia useernamee Ljubljana

upgrade status reports 2 issues, with only the second one being really important (add ^11 to the info.yml)
applying patch: https://www.drupal.org/files/issues/2024-03-18/media_entity_pinterest.8.... and creating a new release is all that needs to be done.

Media entity Pinterest,  8.x-2.7
Scanned on Fri, 07/11/2025 - 14:02

FILE: web/modules/contrib/media_entity_pinterest/media_entity_pinterest.install

STATUS         LINE                           MESSAGE                           
--------------------------------------------------------------------------------
Ignore         29   Fetching deprecated class constant EXISTS_ERROR of interface
                    Drupal\Core\File\FileSystemInterface. Deprecated in         
                    drupal:10.3.0 and is removed from drupal:12.0.0. Use        
                    Drupal\Core\File\FileExists::Error instead.                 
--------------------------------------------------------------------------------

FILE: web/modules/contrib/media_entity_pinterest/media_entity_pinterest.info.yml

STATUS         LINE                           MESSAGE                           
--------------------------------------------------------------------------------
Check manually 5    Value of core_version_requirement: ^9 || ^10 is not         
                    compatible with the next major version of Drupal core. See  
                    https://drupal.org/node/3070687.                            
--------------------------------------------------------------------------------
🇸🇮Slovenia useernamee Ljubljana

Upgrade status does not report any incompatibilities except for adding the 11 into the info.yml. Looks like it only needs merge and new release for d11 compatibility.

🇸🇮Slovenia useernamee Ljubljana

Looks like a just new revision revision is needed to roll out d11 compatibility.

🇸🇮Slovenia useernamee Ljubljana

@fago, I approved the PR, although the comments could be improved. I added some comments to the MR.

🇸🇮Slovenia useernamee Ljubljana

The code looks good. I added a cosmetic comment to display the actual ids of the ce displays that were changed instead of count.

🇸🇮Slovenia useernamee Ljubljana

I agree that the option B is better. There really is no need for additional config checkmark, it was the first thing that popped into my mind when I was thinking about the PR.

🇸🇮Slovenia useernamee Ljubljana

PR looks good to me.

For BC we could add an additional checkbox to add fields to CE display. A config option that is only visible if layout builder is checked and disabled by default.

🇸🇮Slovenia useernamee Ljubljana

I'm moving this into RTBC, but one small improvement would be moving all the in one trait.

All the renderPlain phpstan notifications are gone. There are some new ones in tests/src/Kernel/ThunderParagraphRenderMarkupTest.php

🇸🇮Slovenia useernamee Ljubljana

I was getting the error from this ticket during cron runs. after applying the #12 patch the error is gone. RTBC

🇸🇮Slovenia useernamee Ljubljana

@fago tests are failing now.

🇸🇮Slovenia useernamee Ljubljana

If I click on Codespaces button in github repo the code space fails.

If I click on the badge in the README.md it works.

I suspect the issue is that we removed the default devcontainer.json file which was the same as /base_with_nuxt_naked.

Helpful command:
Use Cmd/Ctrl + Shift + P -> View Creation Log to see full logs

🇸🇮Slovenia useernamee Ljubljana

Additional test case (added in last commit) looks good to me.

🇸🇮Slovenia useernamee Ljubljana

I think that MR is fine, I just added a minor comment on a method that could be generalized a bit and used in other contexts/tests as well.

🇸🇮Slovenia useernamee Ljubljana

Looks good. Most of assertions from functional tests are covered (and many new test cases are added, redirect and unsupported) in the new kernel test - I found one that's missing and I'm wondering if it is really needed (Overriding settings parameter is not allowed.).

🇸🇮Slovenia useernamee Ljubljana

I added tests coverage for multi value fields - which explain the result structure and a small fix to the formatter code.

The header formatter configuration is only there for the FE to know if it should style the first row/column as header. On field configuration there's additional setting called `ignore_table_header` which again still passes through the first row - the config option is only used to determine whether the field is empty if contains only the first row.

🇸🇮Slovenia useernamee Ljubljana

Tested locally and it fixes the issue described in the ticket.

**offtopic** I had some issues testing it locally with the layour_builder integration. Which I had to disable to test.

🇸🇮Slovenia useernamee Ljubljana

One things that needs a check is that ContentFormatCacheContext needs to implement an interface.

Kernel test is passing.

🇸🇮Slovenia useernamee Ljubljana

I removed the label and added the kernel test which was passing locally. I was checking the CI and I think that tests are not running there.

I was unsure about the header settings.

🇸🇮Slovenia useernamee Ljubljana

I reviewed the code and approved the PR.

I tested it locally and it works as it should.

Moving the ticket to RTBC.

🇸🇮Slovenia useernamee Ljubljana

@ajayyadav thank you for your response but when you render the field as custom elements there currently is no 'export as csv' option.

Anyways I had to adjust the processor to support multivalue fields as well, so there's another level of nesting added, but the output I think works out.

🇸🇮Slovenia useernamee Ljubljana

hrefLangcode is generally needed. If the the URL language detection is in place than hrefLangcode should be used as a prefix before path alias to make the path work.

🇸🇮Slovenia useernamee Ljubljana

It would be nice to have operation to open backend api similar as we have for nodes and webform fox DX.

🇸🇮Slovenia useernamee Ljubljana

I approved the PR. Should we document ce name drupal-view-{VIEW}-{DISPLAY_TYPE} somewhere?

🇸🇮Slovenia useernamee Ljubljana

I checked the code and I'm wondering whether there is no nicer way to get the plugin type than substracting the prefix from the plugin id

    // Remove "custom_elements_" prefix from display type.
    $display_type = strpos($view->getDisplay()
       ->getPluginId(), 'custom_elements_') === 0 ?
          substr($view->getDisplay()->getPluginId(), 16) :
          $view->getDisplay()->getPluginId();
🇸🇮Slovenia useernamee Ljubljana

Generally I'd argue that tests for specific functionality that a module provides should live in that module. We have however broken that pattern at the beginning with the general test and later on with the views integration.

For general tests:
- https://git.drupalcode.org/project/lupus_decoupled/-/blob/1.x/tests/src/...
- https://git.drupalcode.org/project/lupus_decoupled/-/blob/1.x/tests/src/...
I think it is fine if they stay on the top level module, but the views tests should go into the lupus_decoupled_views submodule.

🇸🇮Slovenia useernamee Ljubljana

Thank you for review @arthur. I've created a new lupus-decoupled-project PR.

🇸🇮Slovenia useernamee Ljubljana

Follow up tickets:

I'd like to open the discussion on removing the gitpod URLs from README and documentation. gitpod.io links are still working and devcontainer standard is supported by the gitpod flex so I guess we don't have to remove it completely.

🇸🇮Slovenia useernamee Ljubljana

Thank you @arthur. I wasn't paying attention to caching.

- Added cacheable dependencies to the response.
- Cache is tested.

🇸🇮Slovenia useernamee Ljubljana

I removed the missing config option and added a watchdog error for that case instead:

{"system.site":{"name":"Drush Site-Install","slogan":"lupus.digital Publishing","mail":"admin@example.com","missing":null},"missing":{"missing":null}}
🇸🇮Slovenia useernamee Ljubljana

Confirmed that the MR fixes the issue.

🇸🇮Slovenia useernamee Ljubljana

> I am not sure about the /api/lupus prefix also, what about just /api/site-info or /lupus-site-info ?

I went with the /api/site-info but this is also a limitation of the Rest resource which I picked for the task, since it would seem nicer to have a resource at the endpoint of /site-info, for uniformity.

🇸🇮Slovenia useernamee Ljubljana

I created the Lupus Decoupled Site Info module and added some initial configuration.

🇸🇮Slovenia useernamee Ljubljana

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

🇸🇮Slovenia useernamee Ljubljana

I think that provided MR works fine, I'd just like to highlight the other way of adding data to ce-api responses - adding the lupus_ce_renderer_response_data request attribute. It could also go into custom code:

  public function addBasicUserInfo() {
    $request = $this->getCurrentRequest();
    $lupus_ce_renderer_response_data = $request->attributes->get('lupus_ce_renderer_response_data');
    $lupus_ce_renderer_response_data['current_user'] = $this->getCurrentUserInfo();
    $request->attributes->set('lupus_ce_renderer_response_data', $lupus_ce_renderer_response_data);
  }

Additionally, there is hook_lupus_ce_renderer_response_alter as well.

🇸🇮Slovenia useernamee Ljubljana

Tested locally and PR works fine, thus moving into RTBC.

🇸🇮Slovenia useernamee Ljubljana

After further investigation I found out that Gitpod Classic is going to be shut down in April 📌 Gitpod Classic will shut down, making Drupalpod obsolete Active .

I did try to override the .gitpod.yml file but wasn't successful (It probably needs to get merged into the develop branch and I'm a bit afraid of testing it there).

maybe we could write instructions how to enable the modules in gitpod terminal:

      ddev composer require "drupal/responsive_preview":"^2.0" "drupal/rest_log": "^2.3" "drupal/schema_metatag":"^3.0" "drupal/webform":"^6.0" "drupal/search_api": "^1.0" "drupal/search_api_exclude": "^2.0"
      ddev drush en lupus_decoupled_responsive_preview
🇸🇮Slovenia useernamee Ljubljana

testing gitpod with standard installation profile.

🇸🇮Slovenia useernamee Ljubljana

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

🇸🇮Slovenia useernamee Ljubljana

I checked into this issue and did not find a nice way to hook into the toUrl method https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

Maybe I wasn't thorough enough but $options parameter on UrlGeneratorInterface seem to be defined in advance: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Routing%2...

🇸🇮Slovenia useernamee Ljubljana

I had issue testing post request response body until I found rewind method:

$post->getBody()->rewind();
🇸🇮Slovenia useernamee Ljubljana

Code looks good & tests are passing.
RTBC

Will merge and create a release.

🇸🇮Slovenia useernamee Ljubljana

- removed the module and moved the tests to the base module.
- updated tests

🇸🇮Slovenia useernamee Ljubljana

I added a MR, since I was already working on ld.

🇸🇮Slovenia useernamee Ljubljana

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

Production build 0.71.5 2024