@reinfate

Applied suggestion
\Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest::testLogin failed, but I highly doubt it relates to these changes since I didn't even touch PHP.
The test-only pipeline still skips the tests for whatever reason, so it's green.

I wonder if we could just get rid of debounce to fix this. (I pushed MR!10265 with that change)
Before the 🐛 CKEditor5 doens't save updated value if form submitted right after the change Active it worked just like without debounce, but with a 400ms delay for each call as mentioned in the issue summary there.

And IMO it just makes more sense, that onChange should be called on each change immediately, and if the user/contrib code needs to throttle that, it is their responsibility, and they will be able to decide how much they want to throttle it.

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

Rebased branch. Simplified tests by @wim leers suggestions.
Test-only changes in the pipeline are green because the pipeline skips the tests for some reason. Although seems like they did run in the default job.

Some random tests failed, but I'm pretty sure it's not related to this issue.

Checked the issue summary in https://www.drupal.org/project/drupal/issues/2821423 🌱 Dealing with unexpected file deletion due to incorrect file usage Active and this seems like a newly-identified bug

reinfate → created an issue.

Added the test coverage for the isFresh function in ComponentLoader

I've removed the yml file change check, it is not needed, since the component schema will be refreshed only after the discovery cache clear anyway.

The test failures seem to be not related to this issue. I've rerun them multiple times on 11.x and 11.1.x branches, and each time different test was failed.

So seems like only \Drupal\Tests\views\Kernel\Handler\FieldRenderedEntityTest::testRenderedEntityWithoutAndWithField failed.
The problem with this test is that it checks that #cache tags and contexts are exact arrays from \Drupal\Tests\views\Kernel\Handler\FieldRenderedEntityTest::assertCacheabilityMetadata. But these render arrays do not have the #cache['keys'] so updated renderer do not remove duplicates from the cache metadata (as it assumes element will not be cached anyway)
So either we can modify the test, or modify Renderer/RenderContext to apply cache to the element when RenderContext size is 1 (as it indicates that it is the top-most element for the current render() call)

Moving to needs review, since I would like to get feedback on this whole issue before proceeding further.

So another attempt for this.

Here is simple reproduction

    $markups = array_fill(0, 20000, 'markups');
    $build = [];
    foreach ($markups as $i => $markup) {
      $build[] = [
        '#markup' => $markup,
        '#cache' => [
          'tags' => [(string) $i],
        ],
      ];
    }
    $render = \Drupal::service('renderer')->render($build);
    return new Response($render);

Rendering this with #cache takes about 5-8 times longer on my machine than without them
Mainly due to array_unique being called 20_000 times on the array that is growing from 1 tag to 20_000 tags

Changing how public methods in Cache and Cacheable object work (to avoid calling array_unique on each cache tag merge) will inevitably fail tests and potentially break some user code.
But that said, I've looked that each CacheBackend from core does another array_unique call before writing cache, so I would guess that even if we remove array_unique call in \Drupal\Core\Cache\Cache merge methods, everything still should work.

But to be safe I've added additional mergeFast methods in the Cache-related classes and made use of them in the Renderer.
Attached screenshot is the profiler isolated to \Drupal::service('renderer')->render($build); call from the example

Lets see if this fail tests

ReINFaTe → changed the visibility of the branch 936984-new-graphic-for to hidden.

ReINFaTe → changed the visibility of the branch 936984-new-graphic-for to active.

ReINFaTe → created an issue.

Reviving this since https://www.drupal.org/project/drupal/issues/3396742 🐛 CKEditor5 doens't save updated value if form submitted right after the change Active was merged.
I made some changes to the test due to the CKEditor version update.

dinazaur → credited ReINFaTe → .

dinazaur → credited ReINFaTe → .

dinazaur → credited ReINFaTe → .

ReINFaTe → created an issue.

ReINFaTe → created an issue.

Hi @jsacksick
Just in case you missed it, you haven't given any credits for the issue.
If it is intended then nevermind.

Created MR, should be a simple fix.

ReINFaTe → created an issue.

https://github.com/nvelychenko/drupal-extend/pull/10

ReINFaTe → created an issue.

I still can manually reproduce the issue on the latest 11.x
So for me, everything remains the same and nothing new is broken.

If it helps, the exact way I reproduce is:
open edit, focus CKEditor field. Place the cursor on the save button. Type something and click save right after. As described in the Issue summary there is only a 400ms window to do that (probably a bit less, including input lag, code execution order etc.)

However, the functional test hits that window consistently, which is why this issue was reported.

Expanded test to include only source editing changes.

It shouldn't fail even without changes from this issue, the one way to make it fail is to bring back the code mentioned in #8 but skip calling callback()

Was it verified that removing this does not cause that bug to return?

The issue with source editing not being saved is basically the same as this one.
The difference is when in "source editing" mode change:data event is called only when switching back or if the form is submitted.
So if only the source was edited and the form was submitted without switching mode back, that 400ms delay mentioned in this issue has no chance of being executed.
Fix from https://www.drupal.org/i/3229174 → were removing delay only for source editing.
With a fix from that issue in either mode on:change has no delay on the first input and everything is good.

I've confirmed manually that the bug from #3229174 has not been reintroduced.
It is testable I believe, but it will be the same test as provided in this issue, only with the mode changed.

It is reproducible here https://ckeditor.com/ckeditor-5/demo/html-support/

1. Open Source editing
2. Insert for example <pre><code class="language-c">int myNum = 15;
   

   in Source editing

3. Switch back to editor mode, select code block, change language to any other
4. Swith back to source. There are now 2 "language" classes in source

The problem occurs only with GHS (so in Drupal only when filter_html is disabled) when the view gets cast to model, it correctly detects language by class, but later instead of replacing it adds a new one.

I haven't found an existing issue about this at GitHub

We had the same issue in a project.
Can confirm that MR changes resolve it. And tests correctly identify an issue.

Since @nod_ already took a look and all threads on MR seem to be resolved, moving to RTBC.

I think str_contains(file_get_contents($fileinfo->getPathname()), '#[') can be moved to catch clause in parseClass. That way most files will not be scanned, and this can be a check to decide if we want to log/throw an error in case parsing fails.

And, I could be wrong, but I think it is okay to throw the critical error in case the file contains an attribute, but the class isn't valid due to whatever. There are no attributes in contrib right now, and that way while someone will be implementing them we can let them know that there shouldn't be discoverable invalid plugins and that they should do something with it (perhaps move the plugin to the submodule with all needed dependencies declared)

There is also a possible problem with logging an error and returning ['id' => NULL, 'content' => NULL]. In case it failed during parse but an error is related to some base class and not the plugin class itself, the plugin will not be rediscovered (even after drush cr) unless something changes in the plugin class. Due to fileCache in getDefinitions. I guess this can happen during development and can cost someone some time to debug.

ReINFaTe → created an issue.

For me, patch #30 doesn't work for image-style tokens like [node:field_image:array:image_style] or [node:field_image:array:entity:image_style]
Patch #36 fixes works for [node:field_image:array:image_style]

Added test for the mentioned issue.
Also, there was the same issue just under a few lines that I fixed as well.
I am uploading the test-only patch as well.

This may be a bug in core that putting isset() is covering up.

@smustgrave There is nothing above, Drupal\jsonapi\Controller\EntityResource->patchIndividual() it is Controller. Also, JSON:API correctly responds to the request without the "id" key in the body. The only issue is PHP warnings.

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

@Wim Leers I've uploaded configs but, AFAIK it works the same way with any text format.
With these configs added empty <i class="anything"></i> tags will be removed. Adding <i> for formats without "filter_html" or <i class> to "Manually editable HTML tags" will fix that and empty <i> tags can be added after that.
\Drupal\Tests\ckeditor5\FunctionalJavascript\SourceEditingEmptyElementTest::testEmptyInlineElement() Works because it adds needed tags as editable.
There is no "bug" only non-obvious behaviour from the user side. As the user, I'm not sure how I am supposed to know to add these to "Manually editable HTML tags" to allow them to be empty, especially when using full_html format which allows everything.
As a developer, I was looking in the code to see that.
\Drupal\ckeditor5\Plugin\CKEditor5Plugin\SourceEditing::getDynamicPluginConfig Most likely this code is responsible for that.

@markagregory It is fixed but

+

In MR!5127 debounce is changed to how it was before https://www.drupal.org/project/ckeditor5/issues/3229174 →
The only thing that changed is "immediate" set to true, so the first execution will be instant. With that changes made to WYSIWYG and source are always submitted.

But I'm not sure if debounce is needed at all, callbacks (web/core/modules/ckeditor5/js/ckeditor5.js:19) store only one function for each CKeditor instance and it is used by Drupal.editorAttach. So callback is only worth executing once and it can be removed.

There is also Drupal.editors.ckeditor5.attachInlineEditor that uses that callback but I don't see where that attach is used at all.

Different behaviour in Drupal.editorDetach, when there are 1 or more text formats, seems weird, but I also don't understand why we need to "Restore the original value if the user didn't make any changes yet", like if there are no changes, that means field already contains original value?

I believe we do not need a test for the debounce/submit issue since there are already failing tests because of that in https://www.drupal.org/project/drupal/issues/3382780#comment-15289456 🐛 [drupalImage] When ckeditor5_arbitraryHtmlSupport is on, fails to update Postponed

Postponed by https://www.drupal.org/project/drupal/issues/3396742 🐛 CKEditor5 doens't save updated value if form submitted right after the change Active

ReINFaTe → created an issue.

I decided to give this another try and found the issue.

This can be reproduced even by bare hands by making the first change to CKEditor after the page reloads and instantly submitting.

The reason why this happens only with 2 or more text formats lies in 2 places
web/core/modules/editor/js/editor.js:332
web/core/modules/editor/src/Element.php:117

The editor is changing the hidden text area before submitting as it is supposed to, but the data attribute isn't changed if the form is submitted right after the change.
The only place where data-editor-value-is-changed is set to false is Element.php with the condition that there is more than 1 format. With the single format that attribute is simply not set and the condition in js is false.

So to fix the test it is enough to add something like waitForElement('css', '[data-editor-value-is-changed="true"]');

But should we create a follow-up to this and fix it, so that there is no way to submit without change applied?

Yes, these failures are legit.
So something weird happens with \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTestBase::testAlignment
It fails if there are 2 text formats created in SetUp, even though the host uses the proper format.
If you add sleep(1) before $page->pressButton('Save') test passes.
Also, it fails if the host value is img or plain text (not wrapped in p)

I had a similar issue when I was writing a new test, I was writing it in the ImageUrlTest class first and tried to save the form to assert that the new image was saved, and it was failing unless I added a sleep(1) call before clicking form submit.

I've no idea why it happens. Seems like submitting works before ckeditor could update the hidden textarea. But why does it happen only when there are 2 text formats?
I could move the test into a separate class that will not inherit ImageTestBase.

Moved the test to ImageUrlTest class and created MR!5124
Test failures from patches seems random

It seems to be a Drupal side issue after all.
It's reproducible only when the text format has an HTML filter disabled (e.g. Full HTML default format).
As far as I understand it has to do with the fact that without HTML filter htmlSupport.GeneralHtmlSupport plugin is enabled and when updating the image it doesn't properly update the image src in the model.

ReINFaTe → created an issue.

Done
Moved the lazy builder callback to the unit test file in MR5009

3 Patches with only fix, only test and both.

The test is only for BigPipe
@catch said ( https://www.drupal.org/project/drupal/issues/3377570#comment-15271705 📌 Add PHP Fibers support to BigPipe RTBC ) that it is not testable, and I've never written tests before, so I'm unsure how bad is it.
I could look at a separate test for Renderer if needed.

ReINFaTe → created an issue.

Found that this has the same issue as described here https://www.drupal.org/project/drupal/issues/3377570#comment-15271179 📌 Add PHP Fibers support to BigPipe RTBC

The fiber loop itself is never suspended due to $iterations being reset at every iteration of the while loop. Changed that in MR!5003

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

I've noticed that fiber loop itself is never suspended due to $iterations being reset at every iteration of the while loop. Pretty sure it was intended to be before the loop.

Also just random thoughts. For example, if we have 5-10 long-running fibers that just waiting for something, each foreach iteration will suspend the current fiber. So it can be very quick suspends in a row, if higher code has nothing to do we will be back here and again quickly suspended 5-10 times. Will it be better to move $iterations check to the while loop to minimize that "spin locking"?

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

It turns out routing is not needed
API notify from Liqpay is handled in \Drupal\commerce_liqpay\Plugin\Commerce\PaymentGateway\Liqpayment::onNotify
And Return page is handled in \Drupal\commerce_liqpay\Plugin\Commerce\PaymentGateway\Liqpayment::onReturn

Therefore I've just removed hook_menu.

The patch is already fully included in 2.0.x

ReINFaTe → created an issue.

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

Oops, wrong patch in 15

#11 throws an exception when creating a new entity with the reference field and IEF simple widget.

I've added a simple condition so it will not try to load the latest revision when the entity is null.

Ready for review
attaching a patch as well.

ReINFaTe → created an issue.

The last patch produces a warning if one of the field ancestors is NULL. For example, if the entity is only being created and the reference field is not set yet.
Fixed it by adding a simple check at the line 136 in the patch

MailjetSyncForm submit function now depends on the MR !38 from https://www.drupal.org/project/mailjet/issues/3346792 🐛 Do mailjet_first_sync in batch Needs review
Added the getMailjetContactListById function to the MailjetHandler service, as I think it can be useful.

I think there should be a more generic function to allow user code to set filters when querying for lists. ~ById, and ~ByName functions can be kept for convenience though.
Maybe it should be in another task. Let me know what you think.

ReINFaTe → created an issue.

ReINFaTe → created an issue.

I did some changes to the PR.
Fork access is bugged for me, so @jitesh_1 please review this one https://git.drupalcode.org/project/commerce_stock_notifications/-/merge_...

@jitesh_1
I was sure that default exception handling would rollback the transaction. But it actually is not.
Updated MR !6

In MR !36 I've changed connection_timeout to 10s.
For me, with the default 2s timeout, it fails pretty often and also fails sometimes with 5s
Not sure if there are any downsides to having a 10s timeout.

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

MR !35 is hopefully a better implementation of the previous patch

@jitesh_1
A database transaction is missing which can lead to losing data.
There is no point to store and reinsert every field, you need the only one that changed.
You need clear data for that field, otherwise Drupal will handle deletion differently and I suppose this can also be an issue.

Once more, please refer to the existing documentation on Drupal.org

Reroll for 3.0.x

@jitesh_1

As for your update, while it can work, probably, I personally don't know about everything Drupal is doing when changing storage definitions nor I can test this on a lot of databases/someone's local patches etc.
I would argue once more, it should be done Drupal way, by deleting old storage, creating new one, and migrating data, in a transaction. That way Drupal would 100% do everything it should, the table can't be left in a broken state etc.

Hi @jitesh_1
Sorry for the delay, a lot of work lately.

About your update function, the field type is changed to timestamp, but the database schema is not updated.
\Drupal\Core\Field\Plugin\Field\FieldType\TimestampItem::schema has the type int, but in the database, it is still varchar(20)
I suppose it's because you did not delete the old storage and Drupal left it as "already existing" web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:1609
You need to delete the old storage definition and create a new one, and also migrate all the old data to it.
It should be done in the transaction, so in case of an error, we don't leave broken data in the database.

@jitesh_1
Okay, go ahead, main thing is to properly update the field without losing old data. And of course, do not break displaying and existing code.

Hi @jitesh_1
As I mentioned above, I'd like not to change type right now and not to force a major release right now.
And even without that you cannot simply change the type in the definition, it's not how it works. Please investigate the field system in Drupal

Mismatched entity and/or field definitions
The following changes were detected in the entity type and field definitions.
Stock notification
The Sent on field needs to be updated

It's from the status page when you change the definition without updating

ReINFaTe → created an issue.

Hi @jitesh_1
I think we shouldn't add new features right now.
First of all, we need to fix the most obvious bugs.
After that, I'd like to not use form_alter for injecting the subscription form into the cart. This approach isn't playing well with the possible custom add-to-cart forms (I experienced that on my own project). It'd be a very big change for our small module. And it can be a problem for anonymous users. Allowing anonymous to subscribe will most likely lead to the need for someone who uses our module to add a captcha to it at least. And we need to do our best so they can do it easily, Handling it for current form alter and future implementation seems like a waste.

Let me know what you think about that.

Seems like we have a bigger problem here
While code that interacts with the sent_time field is working, It has the type of datetime but the timestamp is written into the database and the view fails because it doesn't know it is a timestamp and not a datetime string.
For the time I hacked views data so it thinks it's timestamp. But before the major release, we should fix the type of the sent_time field
@jitesh_1 please review and merge my MR

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

ReINFaTe → created an issue.

ReINFaTe → created an issue.