Account created on 30 May 2008, over 16 years ago
  • Technical Architect at Wunder 
#

Merge Requests

Recent comments

🇫🇮Finland merilainen

Documentation says: "It is recommended to create a free account on rokka.io and get your API key to use the image CDN."
Instead it should say "It's required". Because the install will fail without providing the rokka.io information.
Error: ROKKA_API_KEY is missing in the .env file.

🇫🇮Finland merilainen

Often POST requests require a body in the request. Is it possible now or should I create a ticket for that request?

🇫🇮Finland merilainen

This is still happening. I don't think saving content should rely on display formatting settings and definitely not block saving content.

🇫🇮Finland merilainen

Changes look good and make sense as discussed in Drupal Slack.

🇫🇮Finland merilainen

Define the Conditions class as a Drupal service

- Transformers class will use Conditions in its constructor
- Change itemsToSchema() to non-static
- Call the apply() with $this->conditions->apply($schema, $items); in itemsToSchema()

🇫🇮Finland merilainen

And apparently that broke the rendering, so I will just fake the selection "All" as "One" which works with the if-then-else logic.

🇫🇮Finland merilainen

I had incorrect operator machine name, changed "one" to "xor". Also setting default value didn't work, so I removed that.

🇫🇮Finland merilainen

This will just affect the Drupal UI so that only "oneOf" operator can be selected. The if-then-else (with "not" in the mix) doesn't seem to work easily with "allOf" or anyOf" operators.
Marking as "Needs review" because it would be nice to know if this approach makes sense.

🇫🇮Finland merilainen

This changes the message to be an error displayed on the form element. See the attachment how it looks like.

🇫🇮Finland merilainen

The patch works, after giving the permission for anonymous user I can use it now as anonymous.

I'm not sure if there would be another way than setting the #cache max-age to 0, because it will kill the page cache and for example varnish will not work, making a site vulnerable for DDOS attacks.

🇫🇮Finland merilainen

Changing the patch author to Nathan B.

🇫🇮Finland merilainen

Apparently the empty default values just mess with form validation, so the first patch should be enough. Marking this RTBC.
I'm not sure how this should be done, so I'll just upload the first patch again in this comment.

🇫🇮Finland merilainen

This version will replace most of the conditions from if-then to if-not-else because the if-then else logic seems to require some default values in the schema for the elements. The if-not-else seems to work better and makes validation easier when there are no default values in the elements.

🇫🇮Finland merilainen

This patch takes more drastic approach and removed the use of the non-draft dependencies keyword completely. "Values is" trigger is also handled in the if-then-else structure which seems to work nicely too.
It also changes the getDependencyKey() function so that it will not throw an exception, because it seems to break Webform UI if there is a container element anywhere in a webform. This part needs still more work, because currently it allows to add any element into a condition and many of them do not work correctly.

🇫🇮Finland merilainen

Fix for "Notice: Only variable references should be returned by reference".

🇫🇮Finland merilainen

Yes the v0.8.4 requirement comes from another project and it will probably not work with the older v0.2.1. The problem is actually in the use composer caret ^ symbol, because:
"For pre-1.0 versions it also acts with safety in mind and treats ^0.3 as >=0.3.0 <0.4.0 and ^0.0.3 as >=0.0.3 <0.0.4"
https://getcomposer.org/doc/articles/versions.md#caret-version-range-
So I suppose it would be better to use the tilde ~ instead, so ~v0.2?
"Another way of looking at it is that using ~ specifies a minimum version, but allows the last digit specified to go up."
https://getcomposer.org/doc/articles/versions.md#tilde-version-range-
Here is a new patch with that suggestion.

🇫🇮Finland merilainen

This iteration of the patch adds support for multiple triggers per state and moves the handling of the "filled" trigger from the prepareDependencySchema() to the alterSchemaProperties() so that multiple "filled" triggers can be used per state.
The webformElementStatesAfterBuild() has not been touched yet because I could not get it working on my local.

It starts to feel like major refactoring could be in place, but there is still a lot of handling of the "Value is" trigger which I don't want to touch and might not need to be changed in the end.

🇫🇮Finland merilainen

Yet slightly improved version which uses if-not-then properties instead of if-else. Seems to work better without default values in the schema too.

🇫🇮Finland merilainen

Here is an improved version of the patch which also works with the "empty" trigger, at least to a certain degree.
Having the patch from Default value not available in response Needs review helps in some cases.

🇫🇮Finland merilainen

This patch sets the default value to empty string "" which works better with conditions and seems to also work with numeric fields.

🇫🇮Finland merilainen

Here is another patch which also sets the default value to "false" for checkbox elements when the default value is not set. This seems to work better with react-jsonschema-form and conditions.

🇫🇮Finland merilainen

Here is a patch which will add support for the checked trigger. It works with both "visible" and "required" states.
This approach alters the schema properties and uses the if & then properties instead of using the "dependencies" keyword which is not supported by the latest JSON Schema if I understood correctly.

🇫🇮Finland merilainen

I suddenly got an error while working on a test form:
TypeError: reset(): Argument #1 ($array) must be of type array, string given in reset() (line 254 of /app/drupal/web/modules/contrib/webform_jsonschema/src/Submission.php)
I updated the patch to check that the $dependencies is an array.

🇫🇮Finland merilainen

Here is a patch which will hopefully apply to 6.2.x.

🇫🇮Finland merilainen

I tried to apply the patch, but it would not apply to 6.2.2 nor 6.2.x-dev.

After applying the changes manually, I tested it with a filename which has spaces "test picture with spaces.png".
It worked if I don't select the "Sanitize file name" setting in the element configuration.
If I do select it, I cannot open the file from the results listing, instead I get the following error:
The requested URL "https://my-cms.lndo.site/system/files/webform/test%2520picture%2520with%2520spaces_0.png" was not found on this server.

🇫🇮Finland merilainen

Can you provide steps how to reproduce? I don't get any errors if I add checkboxes element to a webform.

Your patch can also generate a warning about "Undefined index" if the none of the keys in $row['selector']['#options'][$selector] are set.

🇫🇮Finland merilainen

I don't understand the content of the merge request, it doesn't change the library version but instead adds a "self-dependency" to the module itself in composer.json?
https://git.drupalcode.org/project/openai_image_for_drupal/-/merge_reque...

🇫🇮Finland merilainen

Here is a patch to fix the situation. This will also upgrade the openai-php/client version to ^0.8.4.

🇫🇮Finland merilainen

Having trouble with multilingual sites and the proposed fix? This fix in Lando drupal10 recipe should work better:

# Fighting with Styles? This little gem is amazing.
# location ~ ^/sites/.*/files/imagecache/ { # For Drupal <= 6
location ~ ^(/[a-z\-]+)?/sites/.*/files/(css|js|styles)/ { # For Drupal >= 7
    try_files $uri @rewrite;
}
🇫🇮Finland merilainen

There seems to be a Drupal 10 compatible release 2.1.0 https://www.drupal.org/project/link_class/releases/2.1.0
Can this be closed?

🇫🇮Finland merilainen

This is not right, right?

Note! If you do not want to share the configuration of the development modules, you can use the Drupal core functionality from version 10+ onward ...

Here https://www.drupal.org/node/3079028 it says:

Up to Drupal 8.8.0 developers had to rely on contrib solutions such as Config Split ...

Drush 10 is required though.

🇫🇮Finland merilainen

Looks good to me, after patching Upgrade status saysNo problems found.
I will mark this as RTBC as the changes are simple replacements which Drupal Rector could do.

🇫🇮Finland merilainen

But does Drupal core check anything else than missing files defined by the configuration fast_404.paths: '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i'?

From Fast 404 module:

* The second is to check whether or not the URL exists in Drupal by checking
* with the menu router, aliases and redirects. If the page does not exist, we
* will serve a Fast 404 error and exit.

🇫🇮Finland merilainen

Yep, I didn't check the code, I just saw the problem reported by upgrade_status, sorry. This makes it harder to see which projects are actually ready for D10 because I need to manually go through each problem reported by upgrade_status and see if they are false negatives or real problems.

🇫🇮Finland merilainen

The upgrade status module is still complaining about this in some places, but not all. A bug in https://www.drupal.org/project/upgrade_status perhaps?

🇫🇮Finland merilainen

There is also a call to deprecated tmgmt_redirect_queue_dequeue() function in the same function.

🇫🇮Finland merilainen

Now the module seems to give an error when saving a provider:

The website encountered an unexpected error. Please try again later.
TypeError: in_array(): Argument #2 ($haystack) must be of type array, null given in in_array() (line 163 of modules/contrib/tmgmt_ilangl/src/ILangLTranslatorUi.php).

And the data looks like this:

in_array('ilangl', NULL) (Line: 163)
Drupal\tmgmt_ilangl\ILangLTranslatorUi->validateConfigurationForm(Array, Object) (Line: 237)
Drupal\tmgmt\Form\TranslatorForm->validateForm(Array, Object)

Seems that the values are saved to the Drupal state and it's possible to select the provider which the form fetches, so translations still work correctly.

🇫🇮Finland merilainen

This looks better now. Maybe it's a bit drastic change to move everything to Drupal state, because the only thing which might change in the remote is the service_id and services values. Now all other configuration needs to be added manually to each environment.

🇫🇮Finland merilainen

Looks good to me. This will drop Drupal 8 compatibility, but it is not supported anymore anyway.

🇫🇮Finland merilainen

What about problems like
Call to deprecated function file_create_url(). Deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use the appropriate method on \Drupal\Core\File\FileUrlGeneratorInterface instead.

🇫🇮Finland merilainen

What about deprecated code? There are at least two problems which upgrade status reports:

Relying on entity queries to check access by default is deprecated in drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or FALSE to specify whether access should be checked.

in SearchLinks.php and ExtraLinks.php

Production build 0.71.5 2024