🇨🇦Canada @AlexGreen

4. Investigate where FormState::setAnyErrors is used, and/or git history to see if we can figure out when/why it was made static in the first place.

FormState::setAnyErrors and FormState::hasAnyErrors were created in #2225353: Convert $form_state to an object and provide methods like setError() → between comments 19 and 20 by @tim.plunkett in July 2014 for 8.0.x (i.e.: before a Drupal 8.0 release)
No explanation given for why they were made static.

According to Sling Academy guide on PHP static properties

When you declare class properties […] as static, they are accessible without needing to create an instance of the class. This feature is useful for values […] that are logically intrinsic to the class rather than to any individual object

Maybe @tim.plunkett intended for “protected static $anyErrors” to be a “flag” that lasts for the duration of the current page request for the current user? (regarding subforms, inline_entity_form existed for D7 at that point, but it’s unclear if that was considered the use-case).

public static function setAnyErrors() is used in public function setErrorByName() after recording the element with the error and the error message. It’s also used in public function clearErrors() but isn’t defined in an Interface.
public static function hasAnyErrors() is defined in FormStateInterface, and is used ~11 times across Drupal core (as of commit bcabbbdb75 to 11.x on 2024-06-03)

1. 3 times in \Drupal\media_library\Form\AddFormBase and 3 times in \Drupal\Core\Form\FormBuilder to abort processing
2. In \Drupal\Core\Ajax\AjaxFormHelperTrait::ajaxSubmit() to output status messages in an AJAX response
3. In \Drupal\media_library\Form\FileUploadForm::validateUploadElement to abort processing and force the user to re-upload a file
4. In \Drupal\Core\Form\FormStateDecoratorBase::hasAnyErrors() to be a backend for its own \Drupal\Core\Form\FormStateDecoratorBase::hasAnyErrors() function
5. In \Drupal\Tests\Core\Form\FormStateTest::testSetErrorByName() to test setErrorByName()
6. In \Drupal\form_test\Form\FormTestStoragePageCacheForm::validateForm() to know when to cache the form

@alexpott, @tunic, thank you for the feedback.

It sounds like are suggesting the following path forward... do you agree? Anything that I missed?

1. Change substr($callback, 0, 2) == '::' back to str_starts_with($callback, '::') as per @nod_'s suggestion
2. Change cancel and canceled to halt and halted respectively
3. Find some way to abort submitting the form or otherwise prevent running submit handlers when the form state reaches the "halted" state (previously "cancelled"), possibly by changing FormState::setAnyErrors
4. Investigate where FormState::setAnyErrors is used, and/or git history to see if we can figure out when/why it was made static in the first place.
5. See how easy it would be to create a new function in FormState to check if validation was completed without errors, and get other code to use it?

Fixed strict typing and updated the proposed resolution to match how we actually resolved the issue.

Fixed broken tests and added the deprecation warning, ready for re-review :).

Tests are passing, back to RTBC

I can confirm that the code in the branch named 3280279-11 applies to both 11.x (at b20ec5185c) and 10.2.x (at ffd4be4dda)

@longwave I think I understand what you were asking me to do, but I'm still pretty new to this and don't fully understand all the terminology yet.

John Cook → credited AleexGreen → .

Ready for re-review @catch

Last thread resolved, thanks @wim-leers!

Covered reception w/ Klara

I think this is ready for re-review

@catch I have left a comment on the merge request regarding the question raised in your review.

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

@wim-leers, after some manual testing, I figured out that symfony/mime expects to be passed the fully-qualified mime type with subtype and type ( i.e. the Template (second) column in this table) in order to look up the file extensions.
However, CKEditor5 expects to be passed the IANA image type name (i.e. the Name (first) column in the table)

Internally, CKEditor5 adds/removes the 'image/' string to the beginning of the IANA image type to convert between fully-qualified mime types and image types, but as far as I can tell this assumption isn't guaranteed by the IANA, so it may be risky for us to use it too. I'm unsure on how else to achieve this without patching CKEditor5 (i.e. so it always handles fully-qualified mime types) or symfony/mime (i.e. to make it aware of how IANA image type names map to fully-qualified mime types) .

I've made the documentation clear in both cases (filename extension vs image media type name) and the change record. Feedback is welcome :).

Added a draft Change record →

Here is a patch for problem #1.
@wim-leers for problem #2, we adjust ckeditor5_imageUpload.ckeditor5.config.image.upload.types in ckeditor5_test_module_allowed_image_ckeditor5_plugin_info_alter() i.e.: hook_ckeditor5_plugin_info_alter()

Fixed some lints, now ready for review. Feedback welcome!

Sorry, bad patch. Let's see if this one works.

Let's see if Test Bot likes this test.

@smustgrave will add test coverage soon, you'll need some code like this to upload new file types and to be running CKEditor 5:

function my_module_ckeditor5_image_controller_extensions_alter(array &$extensions): void {
  $extensions[] = 'svg';
}

function my_module_ckeditor5_plugin_info_alter(array &$plugin_definitions): void {
  // Add a custom file type to the image upload plugin. Note that 'svg+xml' below
  // should be an IANA media type Name.
  // https://www.iana.org/assignments/media-types/media-types.xhtml#image 
    $imageUploadPlugin = $plugin_definitions['ckeditor5_imageUpload']->toArray();
    $imageUploadPlugin['ckeditor5']['config']['image']['upload']['types'][] = 'svg+xml';
    $plugin_definitions['ckeditor5_imageUpload'] = new \Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition($imageUploadPlugin);
}

See also comments #8, #9 and #10 for why this custom code is necessary

@catch In this case, we think "terminated" is the closer concept that this change is intending to signify. We could change it to setValidationTerminated() if that's desired.

Updated issue summary

Here's a rough draft of how to programmatically allow uploading any type of file to CKEditor5. If the svg_image module implemented this, it would solve @Claudiu.cristea's issue, but also add support for ".webp", ".avif", etc..., as suggested by @wim-leers. The issue title probably needs to be changed now?
This doesn't have any tests yet, I'm just looking for feedback on the general idea.
I did not base this on the previous patch, so I'm not including an interdiff, but if you would like, I can include one :). Feedback welcome.

Note that if you want to get a list of all user accounts, you must use the above query, as simply issuing a GET request to /jsonapi/user/user will result in a HTTP 403 error → .

On Drupal 9 and 10, trying to fetch all users with /jsonapi/user/user no longer returns an HTTP 403 error. It returns the list of users with each user's display names and id. 

Running PHPStan on *all* files.
 ------ ---------------------------------------------------------------- 
  Line   core/tests/Drupal/Tests/Core/Form/FormValidatorTest.php         
 ------ ---------------------------------------------------------------- 
  390    Trying to mock an undefined method element_validate() on class  
         stdClass.                                                       
 ------ ---------------------------------------------------------------- 

Not exactly sure how to fix this error, it was working in #38 and I didn't change this code. @smustgrave

@smustgrave Here is a new patch with the return typehints.

Here is a draft change record, any suggestions/reviews welcome https://www.drupal.org/node/3360541 →

I'll write a change record for this.

@tim.plunkett Is this what you meant?

Oops malformed patch.

To demonstrate that the test in #13 works, here is a patch containing just the test. If my test is effective, this patch should fail, because it does not contain the code to protect against phtml files. I won't be posting an interdiff because this is a test only patch.

Patch applied successfully, but test bot had opinions on spelling and indentation. Here's a new patch generated against 10.1 this time.

Anybody, I like your suggestion of cancelValidation(), I tried implementing it in this patch. Since it is so different from the previous patches, I just wrote it from scratch, so there is no interdiff. (I created this patch on 9.5 because that's how my test environment was setup at the time). The only place there might be a problem (the patch may not apply) is in the test, if that's the case, I'll fix it in short order.

Fixed the test and added one for the new functionality.

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

@smustgrave, there's some code in \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::sanitizeName() which looks like...

if (empty($extensions) || in_array('txt', $extensions, TRUE)) {
  // Add .txt to potentially executable files prior to munging to help prevent
  // exploits. This results in a filenames like filename.php being changed to
  // filename.php.txt prior to munging.
  $filename_parts[] = $final_extension;
  $final_extension = 'txt';
}
else {
  // Since .txt is not an allowed extension do not rename the file. The
  // file will be rejected by file_validate().
  return;
}

... if "txt" is not an allowed extension, it won't be renamed but something down the line (i.e.: file_validate()) will catch it and prevent it from being uploaded. See also some of the other test cases (e.g.: lines 79, 81, and 76 for PHP files).

Also this code worked fine on my local, which was on 10.0.1.

So I'm putting this back to "Needs review".

Updated Issue Summary with more detailed explanation of the problem, proposed resolution, and added steps to reproduce.

I tested the patch in #17 and it applied cleanly and manual testing worked. Brief code review suggest the code is easy to understand and makes good use of the API.

I think keeping the config schema changes is worth it because it would have to make changes to the config schema proposed in 🐛 Missing config schema for block RTBC , i.e. to allow multiple values in the "Default rel" control. This will mean this needs a reroll when 3023172 is merged, but I think that is acceptable.