Fixing a bug where many directives were illegaly allowed to have non-…

…indented content
doctrine · Mar 18, 2021 · 6884c57 · 6884c57
1 parent 66f0727
commit 6884c57
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 2 deletions.
diff --git a/lib/Directives/Directive.php b/lib/Directives/Directive.php
@@ -109,4 +109,30 @@ public function wantCode(): bool
     {
         return false;
     }
+
+    /**
+     * Can this directive apply to content that is not indented under it?
+     *
+     * Most directives that allow content require that content to be
+     * indented under it. For example:
+     *
+     *      .. note::
+     *
+     *          This is my note! It must be indented.
+     *
+     * But some are allowed to apply to content that is *not* indented:
+     *
+     *      .. class:: align-center
+     *
+     *      I will be a "p" tag with an align-center class
+     *
+     * If your directive allows the "class" directive functionality,
+     * return true from this function. The result is that your
+     * directive's process() method will be called for the next
+     * node after your directive (e.g. a ParagraphNode, ListNode, etc)
+     */
+    public function canApplyToNonBlockContent(): bool
+    {
+        return false;
+    }
 }
diff --git a/lib/HTML/Directives/ClassDirective.php b/lib/HTML/Directives/ClassDirective.php
@@ -49,6 +49,11 @@ public function processSub(
         return $document;
     }
 
+    public function canApplyToNonBlockContent(): bool
+    {
+        return true;
+    }
+
     /**
      * @param Node[]   $nodes
      * @param string[] $classes

diff --git a/lib/Parser/DocumentParser.php b/lib/Parser/DocumentParser.php
@@ -84,7 +84,7 @@ class DocumentParser
     /** @var Lines */
     private $lines;
 
-    /** @var integer|null */
+    /** @var int|null */
     private $currentLineNumber;
 
     /** @var string */
@@ -211,6 +211,7 @@ private function parseLines(string $document): void
                 }
             }
         }
+
         $this->currentLineNumber = null;
 
         // DocumentNode is flushed twice to trigger the directives
@@ -283,6 +284,18 @@ private function parseLine(string $line): bool
                         $separatorLineConfig = $this->tableParser->parseTableSeparatorLine($line);
 
                         if ($separatorLineConfig === null) {
+                            if ($this->getCurrentDirective() !== null && ! $this->getCurrentDirective()->canApplyToNonBlockContent()) {
+                                // If there is a directive set, it means we are the line *after* that directive
+                                // But the state is being set to NORMAL, which means we are a non-indented line.
+                                // Some special directive (like class) allow their content to be non-indented.
+                                // But most do not, which means that our directive is now finished.
+                                // We flush so that the directive can be processed. It will be passed a
+                                // null node (We know because we are currently in a NEW state. If there
+                                // had been legitimately-indented content, that would have matched some
+                                // other state (e.g. BLOCK or CODE) and flushed when it finished.
+                                $this->flush();
+                            }
+
                             $this->setState(State::NORMAL);
 
                             return false;
@@ -562,7 +575,7 @@ private function flush(): void
                         'Error while processing "%s" directive%s%s: %s',
                         $currentDirective->getName(),
                         $this->environment->getCurrentFileName() !== '' ? sprintf(' in "%s"', $this->environment->getCurrentFileName()) : '',
-                        $this->currentLineNumber !== null ? (' around line '.$this->currentLineNumber) : '',
+                        $this->currentLineNumber !== null ? ' around line ' . $this->currentLineNumber : '',
                         $e->getMessage()
                     );
 

diff --git a/tests/BuilderWithErrors/BuilderWithErrorsTest.php b/tests/BuilderWithErrors/BuilderWithErrorsTest.php
@@ -6,6 +6,9 @@
 
 use Doctrine\RST\Builder;
 use Doctrine\Tests\RST\BaseBuilderTest;
+use Symfony\Component\DomCrawler\Crawler;
+
+use function trim;
 
 class BuilderWithErrorsTest extends BaseBuilderTest
 {
@@ -17,6 +20,17 @@ protected function configureBuilder(Builder $builder): void
 
     public function testNoContentDirectiveError(): void
     {
+        $contents = $this->getFileContents($this->targetFile('no_content_directive.html'));
+        $crawler  = new Crawler($contents);
+        $bodyHtml = trim($crawler->filter('body')->html());
+
+        // the note is simply left out
+        self::assertSame(<<<EOF
+<p>Testing wrapper node at end of file</p>
+<p>And here is more text.</p>
+EOF
+            , $bodyHtml);
+
         self::assertEquals(
             ['Error while processing "note" directive in "no_content_directive" around line 6: Content expected, none found.'],
             $this->builder->getErrorManager()->getErrors()

diff --git a/tests/BuilderWithErrors/input/no_content_directive.rst b/tests/BuilderWithErrors/input/no_content_directive.rst
@@ -1,3 +1,5 @@
 Testing wrapper node at end of file
 
 .. note::
+
+And here is more text.