@ReINFaTe

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.

Hi @taraskorpach
Thank you for your attribution. I reviewed the code of the module and I think the only issues preventing us from the release are
https://www.drupal.org/project/commerce_stock_notifications/issues/3337086 🐛 Fix passed items to the queue Needs review
https://www.drupal.org/project/commerce_stock_notifications/issues/3333086 🐛 Field "Sent on" is always empty Fixed

After that is fixed we can think about rewriting existing parts before the major release.
Not sure what issue @jitesh_1 is working on and I'm also curious to know what it is.

ReINFaTe → created an issue.

Hi @jitesh_1
After reviewing the changes with @taraskorpach it looks good to me. Feel free to merge if you don't see any issues.

To be on the same page. I think there are some more issues in the code that needs to be fixed, like passing loaded entities into the queue. I will create issues for them later and should fix them the next week. After that, we need to make a new release. Hopefully working release, unlike the current one.

@jitesh_1
You have permission to maintain issues, code and releases.