Account created on 18 May 2018, almost 6 years ago
#

Merge Requests

More

Recent comments

πŸ‡¬πŸ‡·Greece dimitriskr

I've pushed some code changes.
There needs to be a change at \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5TestBase::createNewTextFormat in order to grant the required permission to the current user to allow using each text format. I guess that will make CKEditor5 tests pass and the whole pipeline go green

πŸ‡¬πŸ‡·Greece dimitriskr

Sure, I can give it a go!

Could you also update the remaining tasks in IS?

πŸ‡¬πŸ‡·Greece dimitriskr

Hey mstrelan,

Thank you for looking into this issue
I don't see a commit pushed to this branch/MR

πŸ‡¬πŸ‡·Greece dimitriskr

For ComposerIntegrationTest, I asked in Slack if the replacement was valid, got only one positive response so I committed it.

As for the PagerTest, it came from a related issue and specifically this commit which was reverted there and added here

πŸ‡¬πŸ‡·Greece dimitriskr

Thanks Wim for taking care of this, it was getting difficult for me

1. I put the upgrade path to the tasks, since I thought it was necessary. Since #53, it was made clear it's not necessary, so we can remove it

2. Is it possible to point a URL to /admin/people/permissions, in the filter module permissions so they can be set directly from there? Or just tell people that in Drupal 11, in order to set permissions for the text formats, you must go to the URL above, in the release notes. And add a note in the current field that in 11 this will change

πŸ‡¬πŸ‡·Greece dimitriskr

The failures in the Functional Tests mean that the test user has no access in ckeditor5.upload_image so it cannot upload the image.

We have to set to the user the permission to use the text format, with maybe the following? Otherwise the tests will fail. (tested locally)

    $format_permission = FilterFormat::load('basic_html')->getPermissionName();
    user_role_grant_permissions(RoleInterface::AUTHENTICATED_ID, [$format_permission]);

Or, if we move the calls of $this->createBasicFormat(); to setUp() (since it's called in every test method + in inherited ImageUploadAccessTest), we can add the permission as parameter in drupalCreateUser()

As for the FunctionalJavascript fails, I have no idea what is going on.

πŸ‡¬πŸ‡·Greece dimitriskr

I've removed the expectDeprecation calls in the tests, as also remove the roles property in the formats

I've kept it in \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest since it loads config from a folder called ckeditor4_config and don't want to mess up anything there. If it's ok to remove it, let me know.

The deprecation message testing already exists in \Drupal\Tests\filter\Kernel\FilterLegacyTest

πŸ‡¬πŸ‡·Greece dimitriskr

Alright, I'll continue on this.

And what about the upgrade path? Don't we need to change the configuration of the existing installations?
I was thinking about a hook_post_update (since it's configuration) and load all filters and remove the roles property. Would this be enough?

If there is an example on testing the upgrade path, I could add this too.

πŸ‡¬πŸ‡·Greece dimitriskr

dimitriskr β†’ made their first commit to this issue’s fork.

πŸ‡¬πŸ‡·Greece dimitriskr

I've injected the plugin managers to DisplayPluginBase, in the related issue, so I think if doing πŸ“Œ Inject various dependencies into DisplayPluginBase Needs review , this must be done too.

Luckily it requires only 2 plugin managers, so it should be quick :)

πŸ‡¬πŸ‡·Greece dimitriskr

Finally, ready for review.

P.S. I've deliberatly put the issue node instead of the draft CR link to the trigger_error(), per a conversation with @berdir at Slack.

πŸ‡¬πŸ‡·Greece dimitriskr

Well, after the 3rd consecutive retry, the previously failed test passed.. Random? Connected with infrastructure issues?
https://git.drupalcode.org/issue/drupal-2015121/-/jobs/818007

πŸ‡¬πŸ‡·Greece dimitriskr

MariaDB 10.6: https://git.drupalcode.org/issue/drupal-2015121/-/pipelines/95023
MySQL 8: https://git.drupalcode.org/issue/drupal-2015121/-/pipelines/95019

There's something going on with Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest test

πŸ‡¬πŸ‡·Greece dimitriskr

I've also created a draft CR for this change

πŸ‡¬πŸ‡·Greece dimitriskr

dimitriskr β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡¬πŸ‡·Greece dimitriskr

https://hub.docker.com/r/drupalci/mariadb-10.6 includes a fix for the IO problem.

Whoever has this problem in contrib, can you change to that database and see if it still occurs?

πŸ‡¬πŸ‡·Greece dimitriskr

dimitriskr β†’ made their first commit to this issue’s fork.

πŸ‡¬πŸ‡·Greece dimitriskr

Thanks for the review Wim! One question. In some previous issues regarding deprecation, it had to have one test that would test that the deprecation message would pop up.

I'll remove the deprecated config from the test configuration but shouldn't exist one test to test the deprecation message? (by creating a separate test module with the deprecated config for this exact purpose)

πŸ‡¬πŸ‡·Greece dimitriskr

Can someone close the MR 6042 too? Since this issue is committed. Thank you

πŸ‡¬πŸ‡·Greece dimitriskr

dimitriskr β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡¬πŸ‡·Greece dimitriskr

I'm uploading one of the logs and make it more readable due to gitlab's raw output

πŸ‡¬πŸ‡·Greece dimitriskr

For now, 6126 can be closed.
We've found the cause for nightwatch, but we don't know what's happening with phpunit tests. If we see an increase in random phpunit failures, we could, in the end, commit the changes of 6126 to 11.x and monitor all jobs. But due to the fact phpunit failures are not that common, and usually a re-run of the job works, its priority can be considered minor

πŸ‡¬πŸ‡·Greece dimitriskr

I'm reopening this issue as it's a feature that would improve the user experience, when using the field plugin.
I'm also attaching a patch for 2.x codebase

πŸ‡¬πŸ‡·Greece dimitriskr

dimitriskr β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡¬πŸ‡·Greece dimitriskr

Changed the title so people can easily find this issue

πŸ‡¬πŸ‡·Greece dimitriskr

I've rebased the branch to latest 11.x and incorporated some changes from the latest patches, so that someone else can work on this easily and to have automatic tests (which currently pass)

Leaving at NW due to some open comments by @wimleers, @phenaproxima and @smustgrave in the MR + no CR yet

πŸ‡¬πŸ‡·Greece dimitriskr

No, it's just that dww reported one more of the failures, because we gather whatever we can find, in order to find the cause of it.

I have to say that the nightwatch failures do not feel like they have the same cause of failure with PHPUnit tests.. Let's fix them one by one. Also, since in #7 we have a nightwatch along with DB logs, we don't need to enable CI_DEBUG_SERVICES yet. Let's fix the nightwatch jobs, that seem pretty straightforward and then we can check the phpunit ones

πŸ‡¬πŸ‡·Greece dimitriskr

Thanks @smustgrave.
Moreover in πŸ“Œ Deprecate form_get_options() Closed: duplicate it was proposed to remove form_get_options() as there is no usage in core and (then) in contrib. We need to see if we still want to do that here

πŸ‡¬πŸ‡·Greece dimitriskr

It was a PHPStorm inspection that identified the changes and not manually, so I got no regex for you

πŸ‡¬πŸ‡·Greece dimitriskr

In the related issue, there is work done to move it to a new class. If we want to remove it, it should be decided there and proceed with it in the other issue

πŸ‡¬πŸ‡·Greece dimitriskr

Please ignore the change in .gitlab-ci.yml, which is for debugging purposes, all other changes and reported feedback are ready for this issue

πŸ‡¬πŸ‡·Greece dimitriskr

Updated remaining tasks. No other references are found.
I saw that in some previous commits, some preg_match() was switched with str_starts_with(). I didn't check other instances of preg_match() that could be replaces with these new functions.

πŸ‡¬πŸ‡·Greece dimitriskr

I've copied what I wrote in #30 to the CR, in order to better explain how Unicode::truncate behaves with its parameters

πŸ‡¬πŸ‡·Greece dimitriskr

I rebased yesterday but it wasn't enough
I also found more findings

πŸ‡¬πŸ‡·Greece dimitriskr

Attached are two more findings that are already covered by parent issue

πŸ‡¬πŸ‡·Greece dimitriskr

The script in #16 is good for the parent issue. Here it's about arrays and typecasting their values so it might have some differencein performance?

Also I ran the script too and got different results

 (string): 697.98 ms
   strval: 715.63 ms
 Callback: 2,925.63 ms
πŸ‡¬πŸ‡·Greece dimitriskr

I've altered the existing test to support the new format brought by $cache_context->getContext()

πŸ‡¬πŸ‡·Greece dimitriskr

Rebased to latest 11.x
I messed up the commits but all changes are there and postgresql tests pass

πŸ‡¬πŸ‡·Greece dimitriskr

dimitriskr β†’ made their first commit to this issue’s fork.

πŸ‡¬πŸ‡·Greece dimitriskr

Updated remaining tasks in IS. CR needs update for updating drupal versions in body

πŸ‡¬πŸ‡·Greece dimitriskr

Tests finally pass

NR for what's currently in the MR and NW for the upgrade path

πŸ‡¬πŸ‡·Greece dimitriskr

Tests pass. There was a random failure

Production build https://api.contrib.social 0.61.6-2-g546bc20