🇸🇮Slovenia @useernamee

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

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.

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.

looks good

moving to rtbc

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

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

@fago tests are failing now.

@fago: https://git.drupalcode.org/project/lupus_ce_renderer/-/merge_requests/47...

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

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

Looks good to me. RTBC

useernamee → created an issue.

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.

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.).

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.

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.

All PR comments are resolved. RTBC

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

Kernel test is passing.

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.

I reviewed the code and approved the PR.

I tested it locally and it works as it should.

Moving the ticket to RTBC.

@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.

useernamee → created an issue.

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.

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

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

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();

I checked the code and approved the PR.

Code looks good, needs testing.

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.

quick fix: https://github.com/drunomics/lupus-decoupled-project/pull/25/files

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

Follow up tickets:

• 📌 Review and improve processes described in Play online. Active
• 📌 Configure nginx to properly handle $_SERVER['REMOTE_ADDR'] in codespaces Active
• 📌 Add possibility to see ddev commands output in GitHub Codespaces Active

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.

useernamee → created an issue.

useernamee → created an issue.

useernamee → created an issue.

A move towards GitHub Codespaces has been made in:
- https://github.com/drunomics/lupus-decoupled-project/pull/20
- https://github.com/drunomics/lupus-decoupled-project/pull/22

Documentation has to be improved in:
- https://github.com/drunomics/lupus-decoupled-website/pull/109

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

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

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}}

One issue left to review: 📌 Add tests for lupus_decoupled_form Active

Confirmed that the MR fixes the issue.

> 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.

Looks good, will merge.

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

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

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.

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

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

testing gitpod with standard installation profile.

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

- reRolled the PR https://git.drupalcode.org/project/lupus_decoupled/-/merge_requests/112#...
- added PR remarks https://github.com/drunomics/lupus-decoupled-website/pull/94#pullrequest...

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...

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

$post->getBody()->rewind();

I added a sentence to the docs: https://github.com/drunomics/lupus-decoupled-website/pull/94

merged

Code looks good & tests are passing.
RTBC

Will merge and create a release.

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

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

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

If it helps to anyone: I had to update config_split ^2 (from 1.9) and then error went away.

The most recent incident we had on our system was when there was an png formatted image that was wrongfully saved to have a jpg ending. It got resolved quickly but you might as well check if that's the issue on your system as well.

> However, the whole lupus_decoupled_search_api module... contains zero code. It's only dependencies on lupus_decoupled_views + search_api_db, and a README.

It contains tests. And I wouldn't want to move tests to lupus_decoupled_views (or any other place) since modules have different purpose.

> Isn't this better off being a recipe, instead of introducing a new 'empty' module?

It is a recipe (https://git.drupalcode.org/project/drupal_cms/-/blob/1.x/recipes/drupal_...) - we test that recipe works with lupus decoupled.

> Then we can move tests/modules and tests/src in the 'module root' tests/ folder. I see no issue with that.

There are other ways to add search functionality to lupus decoupled (with external service). This only tests that Drupal CMS Search recipe works.

Remove unnecessary exclusion.

Looks good to me, thank you @lavanytalwar & @chandansha for you contributions.

I added a fix for cspell errors, but there's an issue to discuss: There's a spelling error in the permission id (admininister). I just fixed it (instead of ignoring it), since usually it is only admin that is administrating the trusted redirect and they have the permission by default. A proper fix would be to add an update hook to check all roles permissions and check if this permission has to be re-added without a spelling error but since this is a small project I don't think it is necessary.

I'd just add BC break notice to the next release.

Thank you for you contribution.

I think we should also fix the pipelines warnings in this ticket: https://git.drupalcode.org/project/serve_plain_file/-/merge_requests/6/p...

@chandansha thank you for contribution. I think we should also take the opportunity to fix the cspell, phpcs and phpstan warnings in this ticket.

There are some spelling, lint and codesniffer issues. I'd propose these warnings get fixed before it is merged.

@chandansha - thank you for your contribution. I've added some cspell fixes to the code.

@deepali sardana thank you for your contribution but @mohd sahzad already opened a merge request which is a preferred issue workflow and his .gitlab-ci looks more in line with what we're aiming for. It just need some additional code sniffer fixes and fixes of spelling errors.

Looks good, thus merging.
Thank you kul.pratap for you contribution.

useernamee → created an issue.