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

MtJpGraph support added #2979

Merged
merged 1 commit into from Aug 7, 2022
Merged

MtJpGraph support added #2979

merged 1 commit into from Aug 7, 2022

Conversation

f1mishutka
Copy link
Contributor

@f1mishutka f1mishutka commented Aug 2, 2022

This is:

- [X] refactoring

jpgraph/jpgraph package seems abandoned and outdated. There is a maintained fork mitoteam/jpgraph with latest library sources version and php 8.1 support. This PR adds ability to use it instead jpgraph/jpgraph.

We do not want to break existing compatibility with jpgraph/jpgraph, so there is JpGraphRendererBase.php implementation of IRenderer interface added with two descendants JpGraph (old one) and MtJpGraphRenderer (new one).

So now it is possible to use latest jpgraph version with PhpSpreadsheet without additional dancing or manual sources pathing.

  • composer require mitoteam/jpgraph
  • Settings::setChartRenderer(\PhpOffice\PhpSpreadsheet\Chart\Renderer\MtJpGraphRenderer::class);

Changelog and readme updated as well.

@oleibman
Copy link
Collaborator

oleibman commented Aug 2, 2022

I am not really knowledgeable enough about other packages to decide whether this should move forward or not. I will have to defer to @MarkBaker or @PowerKiKi for that. However, I can suggest some changes which will be needed beforehand.

You have to be able to pass Phpstan. The way Jpgraph is implemented causes many Phpstan problems. In phpstan.neon.dist, we hide those problems with the following line:

    excludePaths:
        - src/PhpSpreadsheet/Chart/Renderer/JpGraph.php

I think it would be okay if you added your Renderer code similarly. I don't know if that's a good idea long-term, but we might be able to evaluate how to approach that after we decide that we want to move this forward.

The test Helper/SampleTest.php specifically excludes some Jpgraph tests.

    public function providerSample(): array
    {
        $skipped = [
            'Chart/32_Chart_read_write_PDF.php', // Unfortunately JpGraph is not up to date for latest PHP and raise many warnings
            'Chart/32_Chart_read_write_HTML.php', // idem
            'Chart/35_Chart_render.php', // idem
        ];

We would definitely want to see that these samples work correctly with your code. You would probably have to make changes to them (and remove them from the skipped array), or clone them (without changing skipped), to use your code. These aren't really formal tests, but you can review their output after running the full test suite and confirm that everything looks okay.

@f1mishutka
Copy link
Contributor Author

f1mishutka commented Aug 2, 2022

To make things little bit more clear:

mitoteam/jpgraph is not some JpGraph fork. This is just original JpGraph library sources maintained as composer package. So this is exactly the same what jpgraph/jpgraph package was. Two important differences:

  • latest (and constantly updated!) version of original jpgraph library sources - so we have php 8.1 support
  • different approach to load library and modules in composer autoloader. That is why I implemented separate renderer for it (oh, and because another classes namespace as well).

Renderer code is not changed at all. That is why I moved it all to JpGraphRendererBase class, so there is only protected init() method is overridden in two different implementations. The goal was to keep old code working exactly same way as before allowing to use mitoteam/jpgraph as replacement if required.

> You have to be able to pass Phpstan. The way Jpgraph is implemented causes many Phpstan problems. In phpstan.neon.dist, we hide those problems with the following line

Done. Thank you for suggestion!

We also have confirmation that my code is working well from @rolinger here: mitoteam/jpgraph#7

P.S.: we use both JpGraph and PhpSpreadsheet in our projects but we do not use charts functionality, so there was no intersection for them before now.

@oleibman
Copy link
Collaborator

oleibman commented Aug 7, 2022

Reviewing your change, I see you have enabled the use of mitoteam without actually making it part of the project. That actually seems like a good staged approach - I can take care of integrating it into the test suite later. I have, in the meantime, done so in a test version, and it seems to produce reasonable results. So I am willing to move forward with it as is. However, you need to resolve the merge conflicts before I can do so. (Github won't show me the problem, but I suspect changelog is the culprit.)

@f1mishutka
Copy link
Contributor Author

Reviewing your change, I see you have enabled the use of mitoteam without actually making it part of the project. That actually seems like a good staged approach - I can take care of integrating it into the test suite later.

Yes, the idea was just to give it a try. If this will go well without major problems there will be a possibility to replace outdated jpgraph/jpgraph package completely.

I have, in the meantime, done so in a test version, and it seems to produce reasonable results. So I am willing to move forward with it as is.

Great news, thank you! Now we have at least two confirmations that this approach works.

However, you need to resolve the merge conflicts before I can do so. (Github won't show me the problem, but I suspect changelog is the culprit.)

There was just minor conflict in changelog. Resolved and merged.

@f1mishutka
Copy link
Contributor Author

Also rebased on latest upsteam and squashed all changes into one commit.

@oleibman oleibman merged commit b65ff9f into PHPOffice:master Aug 7, 2022
11 of 12 checks passed
@oleibman
Copy link
Collaborator

oleibman commented Aug 7, 2022

Thank you for your contribution. I look forward to re-enabling the abandoned tests.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this pull request Aug 8, 2022
PR PHPOffice#2979 added support for mitoteam/jpgraph as an alternative to jpgraph/jpgraph. The package jpgraph/jpgraph is abandoned in composer, and the version loaded with composer has been unusable for some time. This PR removes the dev requirement for jpgraph/jpgraph, and adds a dev requirement for mitoteam/jpgraph in its place.

With a usable graph library, a number of tests and samples that had been disabled are now re-enabled. A lot of new functionality has been added to Charts recently. Some of that new code has exposed bugs in JpgraphRendererBase. I have fixed those where I could. A handful of exceptions remain; I will investigate, and hopefully fix, those over time, but I don't feel it is necessary to fix them all before installing this PR - we are already way ahead of the game with the graphs that are working.

Three members had been ignoring code coverage in whole or in part because of the unavailability of a usable graph libray. Code coverage is restored in them. I am relieved to report that, although they aren't completely covered, adding them did not reduce code coverage by much - it is still over 90.4%.

I took a look at JpgraphRendererBase and Phpstan. Phpstan reports 128 problems. When I added some docblocks to correct some of those, the number increased to 284. Sigh. I will investigate over time, but, for now, we will still suppress Phpstan for JpgraphRendererBase.

I do not find a License file for mitoteam. However, there also wasn't one for jpgraph in the first place. Based on that and the discussion in PHPOffice#2996 (mitoteam will be used in exactly the same manner as mpdf), I don't think this is a problem. IANAL.
oleibman added a commit that referenced this pull request Aug 14, 2022
* Replace Dev jpgraph/jpgraph with mitoteam/jpgraph

PR #2979 added support for mitoteam/jpgraph as an alternative to jpgraph/jpgraph. The package jpgraph/jpgraph is abandoned in composer, and the version loaded with composer has been unusable for some time. This PR removes the dev requirement for jpgraph/jpgraph, and adds a dev requirement for mitoteam/jpgraph in its place.

With a usable graph library, a number of tests and samples that had been disabled are now re-enabled. A lot of new functionality has been added to Charts recently. Some of that new code has exposed bugs in JpgraphRendererBase. I have fixed those where I could. A handful of exceptions remain; I will investigate, and hopefully fix, those over time, but I don't feel it is necessary to fix them all before installing this PR - we are already way ahead of the game with the graphs that are working.

Three members had been ignoring code coverage in whole or in part because of the unavailability of a usable graph libray. Code coverage is restored in them. I am relieved to report that, although they aren't completely covered, adding them did not reduce code coverage by much - it is still over 90.4%.

I took a look at JpgraphRendererBase and Phpstan. Phpstan reports 128 problems. When I added some docblocks to correct some of those, the number increased to 284. Sigh. I will investigate over time, but, for now, we will still suppress Phpstan for JpgraphRendererBase.

I do not find a License file for mitoteam. However, there also wasn't one for jpgraph in the first place. Based on that and the discussion in #2996 (mitoteam will be used in exactly the same manner as mpdf), I don't think this is a problem. IANAL.

* PHP 8.2 Problems

Tons of "cannot create dynamic property" deprecations in jpgraph. Disable the test with most of those for now; leave the two with only a handful of messages enabled.

* Correct Failures in 2 Stock Charts

Down to 6 templates on which Render fails.
@f1mishutka
Copy link
Contributor Author

Hi, @oleibman!

Added LICENSE.md file with license notes to mitoteam/jpgraph . Could you please take a look if this enough?

As for PHP8.2: waiting for php-redis package provided for php8.2 for us to be able to test and fix jpgraph under php 8.2 (we use redis a lot in our project). Anyway we plan to release jpgraph with php 8.2 support as soon as php8.2 will be released (we have strong intention to switch to php 8.2 for our project it as soon as possible).

Thank you for your efforts with tests and checks!

@oleibman
Copy link
Collaborator

Thanks for adding license info. I reiterate that IANAL; I am, however, definitely happier having the license available than not. Since your project is not part of the "production" release for PhpSpreadsheet, I think what you specify is definitely sufficient.

When it comes time to test Php8.2, 2 of the samples which I have left enabled (Chart/32_Chart_read_write_HTML.php and the same for PDF) are good test candidates - they work but issue a few messages. Chart/35_Chart_render.php, which I have disabled, issues a ton of messages, so, once the other 2 are working, that would be the next step.

@f1mishutka
Copy link
Contributor Author

Hi, @oleibman!

Just added v10.2.0-beta of mitoteam/jpgraph with patches for PHP 8.2 compatibility. Could you please run tests and let me know if any warnings or deprecation notices left? Thank you.

@oleibman
Copy link
Collaborator

I didn't notice any problems - good job!

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

Successfully merging this pull request may close these issues.

None yet

2 participants