Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Visibility changes for PhpOffice\PhpSpreadsheet\Helper\Html breaks subclasses #3751

Closed
1 of 8 tasks
jwoodhead opened this issue Sep 27, 2023 · 4 comments · Fixed by #3752
Closed
1 of 8 tasks

Visibility changes for PhpOffice\PhpSpreadsheet\Helper\Html breaks subclasses #3751

jwoodhead opened this issue Sep 27, 2023 · 4 comments · Fixed by #3752

Comments

@jwoodhead
Copy link

This is:

- [X] a bug report
- [ ] a feature request
- [ ] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

What is the expected behavior?

The properties/methods on the PhpOffice\PhpSpreadsheet\Helper\Html class were previously set as protected which allowed for implementing custom functionality via a child classes.

What is the current behavior?

The visibility for the properties/methods was changed to private as part of #3080, breaking any child classes that were previously extending it.

What are the steps to reproduce?

<?php

require __DIR__ . '/vendor/autoload.php';

// Create new Spreadsheet object
$spreadsheet = new \PhpOffice\PhpSpreadsheet\Spreadsheet();

// add code that show the issue here...

class ChildHtmlHelper extends \PhpOffice\PhpSpreadsheet\Helper\Html
{
    // this is referring to properties and methods that are no longer valid
    protected function initialise(): void
    {
        parent::initialise();

        $this->startTagCallbacks = array_merge(
            $this->startTagCallbacks,
            [
                'ul' => 'breakTag',
                'li' => 'startLiTag',
            ]
        );
        $this->endTagCallbacks = array_merge(
            $this->endTagCallbacks,
            [
                'ul' => 'breakTag',
                'li' => 'breakTag',
            ]
        );
    }

    protected function startLiTag($tag): void
    {
        $this->stringData .= "\u{00A0}\u{2022} \u{00A0}";
    }
}

$exampleHtml = '<ul><li>List Item 1</li><li>List Item 2</li></ul>';

$richTextObject = (new ChildHtmlHelper)->toRichTextObject($exampleHtml);

$spreadsheet->getActiveSheet()->setCellValue('A1', $richTextObject);

$writer = new \PhpOffice\PhpSpreadsheet\Writer\Xlsx($spreadsheet);

$writer->save('example_excel.xlsx');

What features do you think are causing the issue

  • Reader
  • Writer
  • Styles
  • Data Validations
  • Formula Calculations
  • Charts
  • AutoFilter
  • Form Elements

Does an issue affect all spreadsheet file formats? If not, which formats are affected?

  • XLSX
  • HTML
  • PDF

Which versions of PhpSpreadsheet and PHP are affected?

PhpSpreadsheet versions >1.26.0

@oleibman
Copy link
Collaborator

This is an issue that has arisen, and has not been acted on, before, e.g. see #3348 and especially #1682. Missing from those items is a use case to demonstrate why the visibility change would be helpful. You have provided one (using a different bulleting style than the default for lists), and it seems like a reasonable request.

We could restore the visibility in order for you to achieve your aims. However, we could also just, say, add a property liBullet, add li to the arrays, and add a method which handles list items based on the value in liBullet. That would be simpler and more reliable for us, and probably for you. If there are few cases where you think the current implementation has gaps, that would probably be a better way to go. However, if you think there's lots of gaps in the current implementation, perhaps allowing subclassing is better. If you think the latter is the case, it would be helpful to let me know what you think some of the other missing use cases might be.

@jwoodhead
Copy link
Author

jwoodhead commented Sep 28, 2023

I do agree that the visibility for the methods/properties should not be restored.

There is no handling for lists in the current implementation which is why we implemented our own solution.

Another gap we discovered is handling for tables. We use PhpSpreadsheet with user-generated WYSIWYG content and implemented our own solution to convert tables into a format that is "legible" using non-breaking spaces and line breaks so each cell is on its own line. We consider this to be a bodge but it works for our specific implementation.

Adding support for tables to the library would be nice, but I imagine there isn't going to be a "best" solution for how to render a table as text in a cell.

One option that might work for allowing customization is implementing static methods for adding start or end tag callbacks as closures.

\PhpOffice\PhpSpreadsheet\Helper\Html::addStartTagCallback('tr', function($stringData) {
    return $stringData."\n\u{00A0}";
}

\PhpOffice\PhpSpreadsheet\Helper\Html::addEndTagCallback('table', function($stringData) {
    return $stringData."\n";
}

These closure callbacks would then be used in handleCallback if there is no existing callback. Exposing the stringData would be required but I don't think the other properties like size and bold would need to be modifiable.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Sep 29, 2023
Fix PHPOffice#3751. User wants to add bullets for list items, and possibly handle table rows and other currently unsupported tags where one size might not fit all.
@oleibman
Copy link
Collaborator

Let me know if #3752 meets your needs.

@jwoodhead
Copy link
Author

Yes that solution will work. Thank you for the quick response!

oleibman added a commit that referenced this issue Sep 30, 2023
* Allow Users to Support Additional Tags in Helper/Html

Fix #3751. User wants to add bullets for list items, and possibly handle table rows and other currently unsupported tags where one size might not fit all.

* Update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants