Allow importing the generated XML in a DOMDocument and improving HTML converter output #348

kusabi · 2019-07-11T06:58:02Z

In reference to #344

Added an optional parameter to the HTMLConverter::convert($records, $headers = null) method.

When an array of headers is passed, the HTML table will contain a row of <th> elements.

The code in question goes from...

$reader = Reader::createFromString($csvString);
$reader->setHeaderOffset(0);
$converter = (new HTMLConverter())
        ->table('dimensions-table')
        ->tr('data-record-offset')
        ->td('title');

$html = $converter->convert($reader);

and becomes...

$reader = Reader::createFromString($csvString);
$reader->setHeaderOffset(0);
$converter = (new HTMLConverter())
        ->table('dimensions-table')
        ->tr('data-record-offset')
        ->td('title');

$html = $converter->convert($reader, $reader->getHeader());

nyamsprod · 2019-07-11T08:47:26Z

@kusabi thanks for the PR I see a great deal of thoughts has been put into it so really I appreciate it.

I have some reservations on some UX decision made:

The first one is the use of boolean value. I'm don't like boolean values because they are boolean and can be confusing at best.
I do not want to change how XMLConverter works. As it is this class does exactly what it is supposed to do and any XML client can use the returned XML without any issue so we should not changed this class.
Last but not least, whatever we do we won't have people liking the solution 100% so to me we should catered to the 80% and leave the rest to the more experienced ones who will surely figure out a way around the limitation and basically rebuild they own HTMLConverter based on the XMLConverter.

So what I would proposed instead and in line with my remarks on #344 is the following public API changed:

<?php

HTMLConverter::convert(iterable $records, array $headerRecord = []): string

On usage it would look like this

<?php

$reader = Reader::createFromString($csvString);
$reader->setHeaderOffset(0);
$converter = (new HTMLConverter())
    ->table('table-csv-data', 'users')
    ->tr('data-record-offset')
    ->td('title')
;
$html = $converter->convert($reader, $reader->getHeader());

And if $headerRecord is not empty the resulting table would look like this

<table class="table-csv-data" id="users">
<thead>
<tr>
<th scope="col">firstname</td>
<th scope="col">lastname</td>
<th scope="col">email</td>
</tr>
</thead>
<tbody>
<tr data-record-offset="0">
<td title="firstname">john</td>
<td title="lastname">doe</td>
<td title="email">[email protected]</td>
</tr>
<tr data-record-offset="1">
<td title="firstname">jane</td>
<td title="lastname">doe</td>
<td title="email">[email protected]</td>
</tr>
</tbody>
<tfoot>
<tr>
<th scope="col">firstname</td>
<th scope="col">lastname</td>
<th scope="col">email</td>
</tr>
</tfoot>

What do you think ?

kusabi · 2019-07-11T12:07:25Z

That's sounds fine, I will look into a solution after work.

My only concern with this approach is that without any modification to the XML converter, the HTML converter is going to have to get a little hacky.

If you are OK with this I will look into your proposed approach.

kusabi · 2019-07-11T14:20:01Z

Ok. So attempt number 2. It is a little more concise this way and not nearly as hacky as I though it was going to get.

What do you think?

nyamsprod · 2019-07-12T06:44:49Z

@kusabi I took the liberty to research the HTML5 and DOM spec and also what is capable or not in PHP regarding what we are trying to do. Here's what I did find:

from HTML spec if you add a THEAD section you are required to add a TBODY one (which we don't in your current implementation)
from DOM spec there's no simple way to change the parent node name without some cumbersome technique (limitation from DOM)
the XMLConverter does a great job in checking and validating the generated XML/HTML and we should let it continue doing that (ease of maintainance).

So I came to this conclusion that a better implementation would be to add one public method to XMLConverter.

XMLConverter::import(iterable $records, DOMDocument $doc): DOMElement;

The import method creates a DOMElement related to the given DOMDocument but does not attach it to it. The great part of this addition is that this method can be extracted from the current XMLConverter::convert implementation with ease. (ie: we do not need to re-invent the wheel)

Once this method is extracted/implemented then adding the new functionnality to the HTMLConverter class is a breeze.

HTMLConverter::convert(iterable $records, array $headerRecord = []): string

And here's a pseudo code

public function convert(iterable $records, array $headerRecord = []): string
{
    if ([] === $headerRecords) {
        // current implementation does not change
    }

    $doc = new DOMDocument('1.0');

    $thead = $this->createHeaders('thead', $headerRecord, $doc);
    $tfoot = $this->createHeaders('tfoot', $headerRecord, $doc);
    $tbody = $this->xml_converter->rootElement('tbody')->import($records, $doc);
    $table = $doc->createElement('table');

    $table->setAttribute('class', $this->class_name);
    $table->setAttribute('id', $this->id_value);
    $table->appendChild($thead);
    $table->appendChild($tfoot);
    $table->appendChild($tbody);

    $doc->appendChild($table);

    return $doc->saveHTML($table);
}

private function createHeaders(string $tagName, array $record, DOMDocument $doc): DOMElement
{
    $converter = (new XMLConverter())
                     ->rootElement($tagName)
                     ->recordElement('tr')
                     ->fieldElement('th');
    $node = $converter->import([$record], $doc);
    foreach ($node->getElementsByTagName('th') as $th) {
        $th->setAttribute('scope', 'col');
    }

    return $node;
}

IMHO this is much easier to understand and to implement and we open the door for those who want a different implementation to do it as they wish using the XMLConverter` object.

kusabi · 2019-07-12T09:37:26Z

No problem, switched to your proposed solution.

If any headers or footers are added then the table has a <thead>, <tbody>, <tfoot>.

I also made the footer and headers optional to each other (have one without the other).

Let me know what you think.

nyamsprod · 2019-07-12T09:42:20Z

src/HTMLConverter.php

+        $table = $doc->createElement('table');
+        $table = $this->styleTableElement($table);
+
+        $table = $this->styleTableElement($table);


added twice 😄

doh! removed

nyamsprod · 2019-07-12T09:43:21Z

src/HTMLConverter.php

+     *
+     * @return DOMElement
+     */
+    private function createRecordRow(string $rowTagName, string $elementTagName, array $record, DOMDocument $doc)


you can typehint the return type directly thanks PHP7 😉

added the PHP7 hint but not removed the phpdock hint as I was not sure if you implied that or not?

nyamsprod · 2019-07-12T09:44:00Z

src/HTMLConverter.php

    {
-        /** @var DOMDocument $doc */
-        $doc = $this->xml_converter->convert($records);
+        // If we do not want to add any headers or records, then we can default entirely to the XML converter


This comment is unnecessary the code is self explaining in itself

nyamsprod · 2019-07-12T09:47:03Z

src/HTMLConverter.php

+     *
+     * @return DOMElement
+     */
+    private function styleTableElement(DOMElement $table_element)


return typed whenever you can

nyamsprod · 2019-07-12T09:48:01Z

tests/HTMLConverterTest.php

+            ->tr('data-record-offset')
+        ;
+
+        $html = $converter->convert($records, $records->getHeader());


if you only provide a thead why does the tfoot is present 🤔

Honestly, I assumed that they came as a threesome...
I originally hid them accordingly, would you you prefer I did that again?

yes hides them accordingly ... I prefer this to be explicit. I was unable to find a resource stating for instance that if you use tfoot then thead is required. The only requirement is that if one is present then tbody must be used.

nyamsprod · 2019-07-12T09:50:11Z

tests/XMLConverterTest.php

+        $doc = new DOMDocument('1.0');
+        $element = $converter->import($records, $doc);
+
+        self::assertInstanceOf(DOMDocument::class, $doc);


nyamsprod · 2019-07-12T09:55:06Z

src/HTMLConverter.php

+     *
+     * @return DOMElement
+     */
+    private function createRecordRow(string $rowTagName, string $elementTagName, array $record, DOMDocument $doc) : DOMElement


rename

$rowTagName to $recordTagName

$elementTagName to $fieldTagName

for consistency in naming with the reste of the package.

src/XMLConverter.php

nyamsprod · 2019-07-12T10:06:41Z

src/HTMLConverter.php

+     *
+     * @return DOMElement
+     */
+    private function styleTableElement(DOMElement $table_element) : DOMElement


I don't think styleTableElement needs to return anything the object is already known. 😉

… output of HTMLConverter.

nyamsprod

I'll accept the PR as is since I believe it's feature complete and I'll add your contribution in the next release but I reserved the right to change some small implementation details if needed. But already big thanks to your contribution 🥇

nyamsprod · 2019-07-16T10:29:54Z

@kusabi here's the changes I made to your code after the PR was merged:

made the methods protected and not private because if someone extends the class (which is possible currently it may fails for him - the class should be made final in the next major release to avoid that)
I've changed the protected methods and parameters names to be more inline with the rest of the package (parameter should be snake_cased ...)
Completely replace the use of XMLConverter::convert with XMLConverter::import in the HTMLConverter class.

I hope I'll be able to release the new version 9.3.0 as soon as possible.

nyamsprod self-assigned this Jul 12, 2019

nyamsprod added enhancement version 9.0 labels Jul 12, 2019

nyamsprod reviewed Jul 12, 2019

View reviewed changes

nyamsprod requested changes Jul 12, 2019

View reviewed changes

nyamsprod reviewed Jul 12, 2019

View reviewed changes

nyamsprod requested changes Jul 12, 2019

View reviewed changes

src/XMLConverter.php Show resolved Hide resolved

nyamsprod reviewed Jul 12, 2019

View reviewed changes

Fixes #344. Added the ability to add headers and footers to the table…

cbdcc9b

… output of HTMLConverter.

nyamsprod changed the title ~~Allow overriding the row item tags of row n in the XML and HTML converters~~ Allow importing the generated XML in a DOMDocument and improving HTML converter output Jul 12, 2019

nyamsprod self-requested a review July 12, 2019 10:25

nyamsprod approved these changes Jul 12, 2019

View reviewed changes

nyamsprod merged commit 1865b6e into thephpleague:master Jul 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow importing the generated XML in a DOMDocument and improving HTML converter output #348

Allow importing the generated XML in a DOMDocument and improving HTML converter output #348

kusabi commented Jul 11, 2019 •

edited

Loading

nyamsprod commented Jul 11, 2019

kusabi commented Jul 11, 2019

kusabi commented Jul 11, 2019

nyamsprod commented Jul 12, 2019 •

edited

Loading

kusabi commented Jul 12, 2019

nyamsprod Jul 12, 2019

kusabi Jul 12, 2019

nyamsprod Jul 12, 2019

kusabi Jul 12, 2019

nyamsprod Jul 12, 2019

kusabi Jul 12, 2019

nyamsprod Jul 12, 2019

kusabi Jul 12, 2019

nyamsprod Jul 12, 2019

kusabi Jul 12, 2019

nyamsprod Jul 12, 2019

kusabi Jul 12, 2019

nyamsprod Jul 12, 2019

nyamsprod Jul 12, 2019

kusabi Jul 12, 2019

nyamsprod Jul 12, 2019

kusabi Jul 12, 2019

nyamsprod left a comment

nyamsprod commented Jul 16, 2019

Allow importing the generated XML in a DOMDocument and improving HTML converter output #348

Allow importing the generated XML in a DOMDocument and improving HTML converter output #348

Conversation

kusabi commented Jul 11, 2019 • edited Loading

nyamsprod commented Jul 11, 2019

kusabi commented Jul 11, 2019

kusabi commented Jul 11, 2019

nyamsprod commented Jul 12, 2019 • edited Loading

kusabi commented Jul 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyamsprod left a comment

Choose a reason for hiding this comment

nyamsprod commented Jul 16, 2019

kusabi commented Jul 11, 2019 •

edited

Loading

nyamsprod commented Jul 12, 2019 •

edited

Loading