🇨🇭Switzerland @stefanos.petrakis@gmail.com

This doesn't seem to be a Webform translation permission specific problem.
Easiest way to check that is by trying to install and enable another contrib module.
It seems it has something to do with filesystem permissions

This has been fixed with c65e8992 available in 7.x-1.x, 2.x and 3.x;
an update of the module should fix this

And again, apologies for the confusion in advance, this has already been handled more than a year ago with the introduction of the 7.x-3.x branch and a respective release (7.x-3.0), upgrading should fix the issue for those with PHP 8.2 or newer, don't need to do anything here code-wise.

Here is the relevant commit

https://git.drupalcode.org/project/entity_translation/-/commit/7f4ab1795...

Closing this as work as designed

Looks super easy, apologies for waiting on this so long, going to commit in 7.x-3.x as this would be the PHP >= 8.2 appropriate branch

Good stuff. So far in my/our testing this works and delivers the expected system behavior.

One thing (not a full review) I noticed so far in the actual codes:

    if (isset($connection_options['schema']) && ($connection_options['schema'] !== 'public')) {

should be

    if (isset($connection_options['schema']) && ($connection_options['schema'] !== 'dbo')) {

Offering to maintain as well, I have security coverage clearance and could work together with the members of the community that are actively interested in keep Simple Cron up to date to process the open issues.

Cheers
-- Stefanos

Applied the handy suggestion and then some more.

Thanks for the breakdown regarding Exceptions. And for keeping my brain rolling. :-)
I attempted that \Exception-based refactoring (35c3a164a90184ad3ba0e53520aaa8ba428e6901) for two reasons, aiming to "ease" the code's state:

1. So that there is only one place in the code that restores $text = $saved_text.
2. So that continue-ing to the next iteration of the root loop from inside deeply-nested loops can be done in a more economic way (than any other I could think of). E.g.
   

   
     foreach ($tasks as $task => $pattern) {
         ...
         for ($i = 0; $i < count($chunks); $i++) {
             ...
             if ($open_tag == '') {
               ...
               // This is where we would want the root iteration to abort and continue to the next one,
               // if a preg_ failure occurs
               $chunks[$i] = preg_replace_callback($pattern, $task, $chunks[$i]);
   
   

But, reading the docs again, I figured this is exactly the case for `continue DEPTH`.
So I replaced \Exception with continue and removed some more redundant code defending.

I find the changeset is now as effective and compact as it can be and the new tests capture what the original issue detected as a problem.

Back to NR (tests are green).

Ready for review

Back to needs work, added one more change, rerunning the tests

Tests are green, back to NR

Hey Benji

Regarding:

... add something like this after each call to a preg_ function:

This is now done, via a bit of refactoring, using exception throwing.
Now, all preg_* functions inside _filter_url() will cause reverting to the saved text and moving on to the next iteration.

This should be good to move back into NR once the tests are green.
I am off for the weekend, so, please take charge of the codes if that will help finalizing this issue.

Back to NR, tests are green for all targets
https://git.drupalcode.org/issue/drupal-3182166/-/pipelines/294430

Picking this up for work following #37 and #38 (in solidarity to folks at DCon :-) )

Let's add tests to this one

Thank you sir!
Will commit this and have a look at other open issues now.

This looks good to me as well! Thanks for your help.

You are welcome!

We could drop support for `^8 ||` at this point, I guess?

Done!

Added the GitlabCI yml file, phpunit tests are green, other non-critical tests are failing but that can be fixed in a follow-up issue.

I will move this to RTBC and would like to merge in the next 1-2 days unless a review comes in that blocks that action.

It would be good to have both D11 support as well as (GitlabCI) tests running again.

Picking this up and hijacking it to add the GitlabCI integration

Thanks for this, makes sense.

stefanos.petrakis@gmail.com → made their first commit to this issue’s fork.

This was a drag, so many people involved and I found that the actual content was so difficult to read.
In order to move into Markdown (as the OP suggested), we produced a quite incomprehensible README document.

I don't like this, given the number of people involved, it didn't look as if improved the quality.
Given this result, it's very difficult for me to assign credits, will only provide them to the OP.

I fixed the text as best as I could and will commit this.

stefanos.petrakis@gmail.com → made their first commit to this issue’s fork.

Merging this, GitlabCi is working

Outdated: Will be covered by 📌 Automated Drupal 11 compatibility fixes for webform_translation_permissions Needs review

Hijacking this to add GitlabCI integration

stefanos.petrakis@gmail.com → changed the visibility of the branch 3435690-automated-drupal-11 to hidden.

stefanos.petrakis@gmail.com → made their first commit to this issue’s fork.

Currently from D 10.3 on (and D 11) this should be fixed according to 🐛 [PP-1] CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file. Postponed

Note: According to this comment 🐛 [PP-1] CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file. Postponed :

Patches for 10.1 and 10.2 are available are issue forks within the main issue.

Although the 11.x commit applies cleanly against 10.2, it's not compatible with the API changes introduced in #3375447: Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface as stated in #49, and the 10.2 patch (if applied) should be subsequently removed upon updating to 10.3.

Going to move this to "Outdated" since the reported problem seems to be fixed in core.

Fixed in 📌 Gitlab CI integration and minor code-aesthetic changes Fixed

Added D11 compatibility metadata and fixed tests on Gitlab CI, will merge and release anew.

I am highjacking this issue to add .gitlab-ci.yml and test that integration.
The changes proposed here were not needed as the 2.0.x branch was already D10 compatible.

At least some steps to reproduce?
I tried with a simple webform without AJAX, got a console warning, but, the submission worked.

Alternatively, could you try to replace the following in turnstile.libraries.yml:

    - core/jquery

to

    - core/drupal.ajax

If that solves the problem you are having (related to the console warnings), then I have an idea how to update the PR.

All right, let's work on this more then. Is there more to know apart from disabling AJAX?

I got a small PoC working that may solve the issue(s) reported here.

For the time, Merge request !3 is available for testing if somebody would have the curiosity/time.

I will move this to Needs Review to get some eyes moving, the maintainers can decide to move this back to Postponed if they feel that this issue should follow the outcomes of 🐛 Ajax support / Use behaviors Fixed

stefanos.petrakis → changed the visibility of the branch 3330710-PoC-render-when-empty to active.

stefanos.petrakis → changed the visibility of the branch 3330710-PoC-render-when-empty to hidden.

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

kim.pepper → credited stefanos.petrakis → .

stefanos.petrakis → created an issue.

This small change to require-dev should fix the tests

stefanos.petrakis → created an issue.

Gonna try to write the tests in the next couple of days.

Switching this to the new 3.x-dev branch and picking it up

Fixed, a D10 release coming soon.

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

stefanos.petrakis → created an issue.

Thanks @gisle

I would like to maintain the module because I use it in several of my projects.
For all purposes, I have previously received security coverage opt-in permission.

1. Integrated the provided patch from #3 into a PR
2. Fixed a deprecation error
3. Added a composer.json for easier dependency fetching during installation

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

I added support for 9.5 and that should now solve your problem (which is quite reasonable).

Another option would have been using mglaman/composer-drupal-lenient plugin, adding a composer exception for FUSV:1.5.x, composer-updating to D10 and then composer-updating to FUSV:2.0.x
But that would have been uglier than simply adding the ~9.5 version support part that I did, since the codes are 100% compatible.

Gonna add a minor release in a couple of minutes.

Checking this out now, be back in a couple of minutes with an answer.

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

Gonna solve this in #3354487: Add tests for preprocess hook logic → which adds tests.

stefanos.petrakis → created an issue.

Tests are now green

Tests are now green for both D10.1 and D9.5

stefanos.petrakis → created an issue.

stefanos.petrakis → created an issue.

IMO the change in the exception message from #37 to #38
- does not implement $interface, or its namespace cannot be resolved.
+ does not have an existing class $interface.
is restoring some confusion to the dev/reader, i.e. it does little to inform that the provided class: for the collected service does not exist. The $interface variable will be generated based on the service collector's definition and not the collected service.

Even more, the actual change in code could focus explicitly on this case, e.g.:

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandl
ersPass.php
@@ -178,6 +178,9 @@ protected function processServiceCollectorPass(
array $pass, $consumer_id, Contai
     foreach ($this->tagCache[$tag] ?? [] as $id => $attributes) {
       // Validate the interface.
       $handler = $container->getDefinition($id);
+      if (class_exists($handler->getClass()) === FALSE) {
+        throw new LogicException("Service '$id' for consumer '$con
sumer_id' does not have a corresponding existing class " . $handler
->getClass());
+      }
       if (!is_subclass_of($handler->getClass(), $interface)) {
         throw new LogicException("Service '$id' for consumer '$con
sumer_id' does not implement $interface.");
       }

That would mean a small change in the first one of the new tests to capture the new exception for non-existing classes.
And a small change in the second test, if we stick to the original exception message (Service '$id' for consumer '$consumer_id' does not implement $interface.

Ready for review

Ready for review

Added codes in the branch, won't open the PR yet, since that should trigger an auto.testing run and I would like to know that testing is enabled (it is not at the time of this writing).

stefanos.petrakis → created an issue.

Ah... It's late, I ended up clicking on "Open merge request" by accident. Nevermind, still waiting for the tests for 3.x/D10 to get enabled for the project.

P.S.: And the patch file is unrelated, hiding it.

Added codes in the branch, won't open the PR yet, since that should trigger an auto.testing run and I would like to know that testing is enabled.

stefanos.petrakis → created an issue.

Hey @zcht, I wanted to look into the problem you reported, I didn't manage to reproduce it though. Can you give me a little more information? I tried it on a fresh standard Drupal 9.5.7 on Php8.1 and didn't get any errors. Any other modules that might play a part?

FYI: I tried the different PHP versions here => https://3v4l.org/mhmnn#v5.6.40

using this snippet:

<?php
// example code

ini_set('pcre.backtrack_limit', 1);
$text = "<p>www.test.com</p>";
$chunks = is_null($text) ? array('') : preg_split('/(<.+?>)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
var_dump($chunks);

Unfortunately, this needs to stay at "Needs Work" for the time.

The test introduced here is based on the idea of breaking the preg_* function using pcre.backtrack_limit; this idea as @poker10 observed does not work for any PHP version smaller than 7.3. From PHP7.3 and higher all preg_* functions return bool(FALSE) on failures.

This breaks the new tests due to this part:

    // Split at all tags; ensures that no tags or attributes are processed.
    $chunks = is_null($text) ? array('') : preg_split('/(<.+?>)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

PHP7.2 (and 7,1 and 7,0): preg_split splits the string partially and $chunks is therefore not FALSE; later on the _filter_url() returns a result that differs from the input text.

PHP5.6 (and 5.3 and probably all 5,*): preg_split does not split, but returns the whole string and here $chunks is not FALSE either; later on the _filter_url() returns the whole string as a result by coincidence keeps the tests from breaking.

Ideas for extending the tests to support 5.x and 7,0,1,2 are welcome, I have to think about this some.

Let's have a look at this one too.

Found it, it was the pcre.backtrack_limit messing with preg_replace calls that the pgsql layer was issuing. A little refactoring made lots of sense to keep that effect restricted.

Waiting for the tests to set this back to NR.

Let's clear this, I am gonna investigate it right now.

Thanks @poker10, this is ready for another round.

This one is ready for both 9.5.x and 10.1.x

It looks good.
Maybe we can be more defensive regarding the $extension_path variable?
As in, if still an empty string, do not enter the main loop.

if (!empty($extension_path) && isset($info['components']) && isset($info['components']['autodetect_libraries']) && isset($info['components']['namespaces'])) {

I just released 8.1.4 with support for D10.
@greggles: Will keep changes wrapped in issues. (Didn't do that for updating the README.md and updating the maintainers list, but I hope you are ok with that exception)

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

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

Ready for another round of reviews, I backported any relevant changes from the final D9/10 committed codes.

Ok, cannot run tests in this way, only via enabled automated testing and the MR. Tests still report green on my local, so this should be good to go.

Uploading the patch version of the updated MR for running the test (since auto-testing is disabled).

Tests are failing, and this was targeting the wrong branch.
The b89f2a13 on 8.1.x should be reverted.

Still RTBC, added a MR for those that don't want to use patches.

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

Removed the jquery/once deprecated dependency, modified js codes accordingly to use core/once