Editor::getFilterFormat() should throw domain error if Editor::format not set

I think it'd be better to throw a \DomainException instead.

The test coverage doesn't actually match the issue title. Either the test or the issue title is inaccurate.

👍

1. +++ b/core/modules/editor/src/Entity/Editor.php
   @@ -126,10 +127,15 @@ public function hasAssociatedFilterFormat() {
   -    if (!$this->filterFormat) {
   -      $this->filterFormat = \Drupal::entityTypeManager()->getStorage('filter_format')->load($this->format);
   +    if ($this->filterFormat) {
   +      return $this->filterFormat;
        }
   -    return $this->filterFormat;
   +    elseif (empty($this->format)) {
   +      throw new \DomainException('Editor::getFilterFormat should not be called if Editor lacks an assigned text format.');
   +    }
   +    return \Drupal::entityTypeManager()
   +      ->getStorage('filter_format')
   +      ->load($this->format);
   

   This no longer sets $this->filterFormat... I think

       if (empty($this->format)) {
         throw new \DomainException('Editor::getFilterFormat should not be called if Editor lacks an assigned text format.');
       }
       if (!$this->filterFormat) {
         $this->filterFormat = \Drupal::entityTypeManager()
           ->getStorage('filter_format')
           ->load($this->format);
       }
       return $this->filterFormat;
   

   would work better.

2. +++ b/core/modules/editor/tests/src/Kernel/EditorFilterIntegrationTest.php
   @@ -52,4 +52,23 @@ public function testTextFormatIntegration() {
   +  public function testEmptyFilterFormat() {
   +    $editor = Editor::create(['editor' => 'unicorn']);
   +    $this->expectException(\DomainException::class);
   +    $editor->getFilterFormat();
   +
   +    // Create an arbitrary text format.
   +    $format = FilterFormat::create([
   +      'format' => mb_strtolower($this->randomMachineName()),
   +      'name' => $this->randomString(),
   +    ]);
   +    $format->save();
   +    $editor->set('format', $format->id());
   +    $format = $editor->getFilterFormat();
   +    $this->assertInstanceOf(FilterFormat::class, $format);
   +  }
   

   This is not testing what you think it is. After the exception is thrown nothing else in the test is executed. So if you change assertInstanceOf to assertNotInstanceOf the test still passes.

3. +++ b/core/modules/editor/tests/src/Kernel/EditorFilterIntegrationTest.php
   @@ -52,4 +52,23 @@ public function testTextFormatIntegration() {
   +    $this->expectException(\DomainException::class);
   +    $editor->getFilterFormat();
   +
   

   Should test the exception message too just in case there are multiple domain exceptions.

+++ b/core/modules/editor/src/Entity/Editor.php
@@ -126,8 +127,13 @@ public function hasAssociatedFilterFormat() {
+      throw new \DomainException('Editor::getFilterFormat should not be called if Editor lacks an assigned text format.');

Thinking about this some more... I think including the ID of the editor entity would be helpful. Also can you use __METHOD__ instead of hardcoding the method name. It makes the code a bit more portable.

@oknate unfortunately the credit checkboxes only work for maintainers.

I think You cannot call getFilterFormat() on the editor HelloWorld since it does not have an assigned text format. works for me. Not sure - either version looks fine to be honest. In exception messages you can use sprintf() and not FormattableMarkup() because error mostly not going to HTML (think syslog). So variables are often put in quotes.

@oknate sorry I should have been clearer. I think only the editor ID needs quotes. Other than that this looks great.

1. +++ b/core/modules/editor/src/Entity/Editor.php
   @@ -126,8 +127,13 @@ public function hasAssociatedFilterFormat() {
   +    if (empty($this->format)) {
   +      throw new \DomainException(sprintf('You cannot call %s on the editor "%s" since it does not have an assigned text format.', __METHOD__, $this->getEditor()));
   +    }
   

   I have a feeling that this is going to be contentious. The documentation for EditorInterface::getFilterFormat() explicitly says this:

   This could be NULL if the associated filter format is still being created.

   That makes me think that this behavior, unhelpful though it may be, was intentional and that adding this exception constitutes an API/behavior change. Therefore, I'm tagging this for framework manager review.

2. +++ b/core/modules/editor/src/Entity/Editor.php
   @@ -126,8 +127,13 @@ public function hasAssociatedFilterFormat() {
   -      $this->filterFormat = \Drupal::entityTypeManager()->getStorage('filter_format')->load($this->format);
   +      $this->filterFormat = \Drupal::entityTypeManager()
   +        ->getStorage('filter_format')
   +        ->load($this->format);
   

   This change seems out of scope?

Oh wait -- looking up in the issue, I see that @alexpott already reviewed this patch. If he thought this was okay, I guess this doesn't technically need framework manager review. Still...the interface documentation says something different, so I'm going to keep the tag.

+1 on fixing the docs and throwing a proper exception.

Explicit UI tests were added in #2207777: Can not configure editor whilst creating a new text format, so I think this change won't break/regress as those are still passing here with this change.

This looks good to me. +1 RTBC.

What remains to be done here? This seems very ready per #27, #28 and #29? 🤔

Agree with #33, there seems nothing left to do here, the patch is easy to follow and adds a test. The patch still applies cleanly to 9.3.x as well.

+++ b/core/modules/editor/src/EditorInterface.php
@@ -23,10 +23,10 @@ public function hasAssociatedFilterFormat();
    *
-   * @return \Drupal\filter\FilterFormatInterface|null
+   * @throws \DomainException
+   *   If called without an associated filter format.
    */

I think this could do with a bit more explanation why the editor wouldn't have an associate format (only when it's being created?) (which the original comment being removed has).

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

#35 still needs to be addressed
Tagging for change record so others are aware of the new change.

Rest looks good!

I think this would be a good novice issue 😊

Any takers? I promise reviews :)

Created the MR using the #26 patch and addressed comment #35 as well, but the pipeline failed. keeping it on "Needs work".., please review the changes.

Change as per #35:

   /**
    * Returns the filter format this text editor is associated with.
    *
    * @return \Drupal\filter\FilterFormatInterface
    *
    * @throws \DomainException
 -  *   If called without an associated filter format.
 +  *   Thrown if the text editor is called without an associated filter format.
    */
  public function getFilterFormat();

Those are all broken tests. Each of them is creating an Editor config entity without a corresponding FilterFormat already existing. We should fix those tests.

Updating the remaining tasks to point to @Wim Leers' feedback. Thanks!

I looked into updating the tests, specifically the failing tests from
core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest::testInvalidPluginDefinitions
which calls
core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest::providerTestInvalidPluginDefinitions.

The provider returns YAML with invalid ckeditor5 plugin definitions. The way I understand it, this YAML should define a FilterFormat that the ckeditor5 Editor uses. But how... that is not clear to me. Anyone have an idea on how this is done?