Account created on 24 February 2010, over 15 years ago
#

Merge Requests

More

Recent comments

🇪🇸Spain isholgueras

Yesteday I've created this issue and MR: https://www.drupal.org/project/canvas/issues/3551787 📌 Re-introduce entityId and entityType into drupalSettings.canvas Active after a chat with Balint.

🇪🇸Spain isholgueras

Tugboat is also broken because DDS is using a lot of canvas.module/textarea

I've created an Issue: https://www.drupal.org/project/demo_design_system/issues/3552037 🐛 textarea is not a valid json_schema anymore Active and a MR here: https://git.drupalcode.org/project/demo_design_system/-/merge_requests/114

This MR, hopefully, fixes the Tugboat issue.

🇪🇸Spain isholgueras

I've created this issue because in the 📌 Update Conductor to Canvas RC Active issue we've found that these 2 variables no longer exists in RC1.

I've seen that there is only one route processed (besides the /canvas/api), and is /canvas. This route is canvas.boot.empty and this route doesn't have any parameters. All the /canvas routes are processed in the CanvasPathProcessor

Is this the expected behavior? If so, CanvasController::__invoke will always have null parameters for entity and entity_type?

🇪🇸Spain isholgueras

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

🇪🇸Spain isholgueras

I think this is no longer an issue, or at least I'm unable to reproduce it with the steps.

The drupalSettings.conductor.accountId is set in a js_settings_alter, but originally was set in a js_settings_build.

I've changed it to the js_settings_alter because I had some issues with cache, but I think that's now fixed.

🇪🇸Spain isholgueras

After seeing that there is a new release for DDS that handle the Canvas rename, and after removing the canvas_dev_standard module as canvas does, it's ready for review and merge.

🇪🇸Spain isholgueras

After applying the rename on 📌 Update XB occurrences to Canvas Postponed , Tugboat was merged without changing it to canvas.

🇪🇸Spain isholgueras

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

🇪🇸Spain isholgueras

I've tested it in 2 different Drupal 11
- An existing Drupal 11 with Canvas and Conductor installed and configured. Replaced the existing module by the zip version, cleared caches and everything work.
- A basic Drupal 11 with Canvas. Reinstall the site, place conductor into web/modules/contrib/conductor and install it. Everything works as expected too.

RTBC for me.

🇪🇸Spain isholgueras

If the response from conductor has the Content-Type header with more attributes, like application/json; charset=UTF-8, the json is not correctly parsed.

🇪🇸Spain isholgueras

Thanks for the work @Rajeshreeputra! I'm going take your job and continue it.

🇪🇸Spain isholgueras

I've reviewed the code, looks good. I've also reviewed the functionality and it works as expected.

For me is a RTBC

🇪🇸Spain isholgueras

I think it's ready for a first review.

The steps to test is:
- Enable the module
- Add to the config page the api key and secret
- Make requests to the conductor api resources like using the route /conductor/api/{conductor-resource}
- /conductor/api/accounts

It should accept GET, PUT, POST and DELETE requests.

🇪🇸Spain isholgueras

Core is currently doing for /system/files/ route.

It uses an InboundPathProcessor to get the route of the file, set it to a query parameter and return just the /system/files to allow the route system to match the route, no matter how many / the file has, because now is in a query parameter.

class PathProcessorFiles implements InboundPathProcessorInterface {

  /**
   * {@inheritdoc}
   */
  public function processInbound($path, Request $request) {
    if (str_starts_with($path, '/system/files/') && !$request->query->has('file')) {
      $file_path = preg_replace('|^\/system\/files\/|', '', $path);
      $request->query->set('file', $file_path);
      return '/system/files';
    }
    return $path;
  }

}

Then, in the controller, it gets the target by accessing the file query parameter.

    $target = $request->query->get('file');
🇪🇸Spain isholgueras

I have one idea to write a test for this. If it works I think it will be easier to refactor this.

🇪🇸Spain isholgueras

Is this still an issue? I've went through:

1. Install Standard install profile
2. Install experience_builder
3. Create article node — let's assume it's node 1
4. Install xb_dev_standard
5. Go to /node/1 and ...
... add a new image (without the media module)

6. Click "Review 1 change" and "Publish"
7. Visit /node/1 and see my image

I can investigate deeper on when this was fixed, but I think this can be closed.

🇪🇸Spain isholgueras

I've added some concerns in the MR about duplicated conditions and code that can be removed.

🇪🇸Spain isholgueras

The only difference that this MR brings is the exception logged in the database and is step forward and we would have to delete this sooner or later. I'm OK with this.

I've fixed one phpunit test and all should be green now.

🇪🇸Spain isholgueras

Oh, I see it now. Ok, make sense then. I thought it was leftover code.

No, it doesn't make sense to rename it. I think it can be closed.

Thanks!

🇪🇸Spain isholgueras

After talking to @wim, I've realized that #10 📌 Move `PropSourceEndpointTest` into new `XbConfigEntityHttpApiTest::testComponent()` Active is adding a new task, so it's not ready.

The current MR1379 is only moving the test

🇪🇸Spain isholgueras

isholgueras changed the visibility of the branch 3510896-move-propsourceendpointtest-into to hidden.

🇪🇸Spain isholgueras

isholgueras changed the visibility of the branch 3510896-move-propsourceendpointtest-into to hidden.

🇪🇸Spain isholgueras

This project is currently configuring XB under Drupal CMS: https://github.com/phenaproxima/xb-demo

To have XB working, you need to add a specific xb field type component_tree under article or xb_page. No other content types are currently supported, and this project does that.

Here is the ticket where the conversation about supporting other content types is happening: 📌 Allow XB to be used on all node types Active

🇪🇸Spain isholgueras

If the issue is the

          $component = Component::load($component_instance['component']);
          assert($component instanceof Component);

Could we change only this assert with a regular if if we're in preview? Something like this draft code:

          $component = Component::load($component_instance['component']);
          // Avoid asserts in preview with zend.assertions=1 to improve message to users.
          if (!$isPreview) {
            assert($component instanceof Component);
          }
          elseif (!$component instanceof Component) {
            // Or anything else.
            throw new ComponentNotFoundException($component_instance['component'] . ' doesn\'t exist.');
          }

I'm probably missing something but that's the only assert we need to sort, right?

🇪🇸Spain isholgueras

After reviewing both MR, I think I feel mor comfortable with MR 1351 (ignoring changed). https://git.drupalcode.org/project/experience_builder/-/merge_requests/1351.

It doesn't require any front-end changes but it would probably be good idea for the front-end to also stop sending changed but it could be in follow-up

Agree, but I think you're covering it well by ignoring changed.

🇪🇸Spain isholgueras

After debugging a bit, I think it's because of this piece of code in ClientDataToEntityConverter

https://git.drupalcode.org/issue/experience_builder-3536247/-/blob/35362...

      if ($entity instanceof EntityChangedInterface && $name === 'changed') {
        $changed_timestamp = $items->first()?->get('value');
        assert($changed_timestamp instanceof Timestamp);
        $changed_timestamp_int = $changed_timestamp->getCastedValue();
        assert(is_int($changed_timestamp_int));
        $form_updated_changed_field = $changed_timestamp_int !== ((int) $entity_form_fields['changed']);
      }
    }

    assert(!is_null($entity->id()));
    $original_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadUnchanged($entity->id());
    assert($original_entity instanceof FieldableEntityInterface);
    // Filter out form_build_id, form_id and form_token.
    $entity_form_fields = array_filter($entity_form_fields, static fn (string|int $key): bool => is_string($key) && $entity->hasField($key), ARRAY_FILTER_USE_KEY);
    // Copied from \Drupal\jsonapi\Controller\EntityResource::updateEntityField()
    // but with the additional special-casing for `changed`.
    foreach ($entity_form_fields as $field_name => $field_value) {
      \assert(\is_string($field_name));
      if ($field_name === 'changed' && $form_updated_changed_field) {
        continue;
      }
      try {
        $original_field = $original_entity->get($field_name);

There is a case when the $form_updated_changed_field value is not TRUE so is set to the original field, and the opposite. I still working on how to reproduce it reliably.

🇪🇸Spain isholgueras

I've left some minor comments. Overall, It looks good to me though

🇪🇸Spain isholgueras

We decided that we're going to validate only the entities ($entity->validate) right before the $entity->save() to still validate Constraints and don't execute the form validate.

I'll work on that and adapt the tests.

Production build 0.71.5 2024