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

Make methods strongly typed instead of using string input parameters #1669

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0b10011
Copy link
Contributor

@0b10011 0b10011 commented Jul 18, 2019

This is ready to merge, pending any review from you. (Change log may need updated depending on timing of next release.)

Description

PHPWord uses a mixture of strings (constants and otherwise), integers, floats, and magic to track widths, heights, colors, styles, etc. Unfortunately, it's currently impossible to check the correct units are used, and colors and styles are often not validated.

This is an attempt to move PHPWord to a more OOP codebase, where input is verified, lengths can be provided using any unit, and conversion for import/export happens automatically.

For example, currently, you can add a row to a table with $table->addRow(900). However, there's no unit provided with the height and the documentation for addRow doesn't list which unit should be used. While the unit could be listed in the documentation fairly easily, that doesn't help to avoid user errors. With this new approach, adding a row would use $table->addRow(Length::from("twip", 900)). It is now easy to see which unit is used from the calling code, and the receiving code no longer has to worry about which unit was provided, as it can use $height->toInt("twip"), guaranteeing the correct unit is used.

Added

  • Strict types throughout codebase
  • Return type declarations
  • Length validation and automatic unit conversion with PhpOffice\PhpWord\Style\Lengths\{Absolute, Auto, Percent}
  • Color validation with PhpOffice\PhpWord\Style\Colors\{BasicColor, SpecialColor}
  • BorderStyle validation with PhpOffice\PhpWord\Style\BorderStyle
  • Support for additional border-style values (hidden, groove, ridge, inset, and outset) in HTML documents
  • Support for all named colors from CSS in HTML documents
  • Support for borders on Cell
  • Support for writing percent and auto column widths on Table to ODT files
  • Support for space and shadow on Border (and support for writing to Word2007)

Changed

  • float and int are no longer supported for lengths. Only PhpOffice\PhpWord\Style\Lengths\{Absolute, Auto, Percent} are allowed.
  • string is no longer supported for colors. Only PhpOffice\PhpWord\Style\Colors\{BasicColor, SpecialColor} is allowed.
  • string is no longer supported for border styles. Only PhpOffice\PhpWord\Style\BorderStyle is allowed.

Fixed

  • Fixed specifying cell widths, background color, etc on PhpOffice\PhpWord\Style\Cell
  • Escape arrays of replacements in TemplateProcessor

Checklist:

  • I have run composer run-script check --timeout=0 and no errors were reported
  • The new code is covered by unit tests (check build/coverage for coverage report)
  • I have updated the documentation to describe the changes

Other todo

  • Double check documentation has been fully updated for latest changes
  • Go through open issues and see if any have been fixed by this
  • Update CHANGELOG

Concerns

  • This is not backward compatible. Lengths, colors, and border styles require associated objects instead of integers, floats, and/or strings. The API has remained essentially the same other than that though.
  • Some code that was shared previously no longer can be due to the output requiring different units (eg, eops vs twips for some borders).
  • This is a big change, and very likely introduced some issues that aren't tested. And to fully get the library up to current standards, there's more work to be done. But I've got the tests passing, and am fixing any untested issues I come across as I find them.
  • A lot of code is "covered" by tests that don't actually test any those values. The library could go for a lot of @covers annotations to be added. I haven't done this in this commit as I'm trying to keep this as clean of a merge as I can for such a large change. I plan on doing followup PRs with more changes (such as adding proper tests for pseudo-covered code).

@0b10011 0b10011 force-pushed the oop branch 24 times, most recently from a3b03e3 to c67da2e Compare July 25, 2019 16:47
@coveralls
Copy link

coveralls commented Jul 25, 2019

Coverage Status

Coverage increased (+0.2%) to 94.814% when pulling fe3e45b on 0b10011:oop into 8fbd060 on PHPOffice:develop.

@0b10011 0b10011 force-pushed the oop branch 5 times, most recently from d8f3c44 to f8b5390 Compare July 29, 2019 14:13
@0b10011
Copy link
Contributor Author

0b10011 commented Aug 28, 2019

OS issue has been fixed by updating to xenial from precise. Since PHP has already been bumped to 7.0 in this PR, this was a non-issue.

@0b10011
Copy link
Contributor Author

0b10011 commented Aug 28, 2019

I've updated documentation and samples with most recent changes (and some fixes).

Only thing left is the CHANGELOG. I've added some changes to it already, but I'll go through it one more time. However, that depends on #1689. I'm assuming (and would prefer so I can get more fixes in) that you want to put out a release before merging such a large change, in which case I can move all of those changes to the next version in the change log. Just let me know!

@0b10011
Copy link
Contributor Author

0b10011 commented Aug 28, 2019

CI and Scrutinizer both pass, and last I checked, the third test passed as well. This should all be good to go, pending your review. I shouldn't be pushing any more commits unless requested from you. If you want the full commit history (instead of squashed commits), I can force push that instead, but be aware that Scrutinizer won't work with it due to too many commits in a single PR.

@0b10011
Copy link
Contributor Author

0b10011 commented Sep 2, 2019

Merging #1661 broke this. I'll get a fixed version up tomorrow. (Tried using GitHub's in-browser conflict resolution, but it resulted in an empty commit.)

@0b10011
Copy link
Contributor Author

0b10011 commented Sep 3, 2019

Rebased, fixed, and squashed. All checks passing.

@troosan
Copy link
Contributor

troosan commented Sep 30, 2019

@0b10011 Impressive work. I'm just a bit worried about the non-backward compatibility of the changes.
This one will make it very difficult to migrate to a next version. It would be nice to be able to still pass sizes/colors/... the "old" way, and transform internally.

@troosan troosan added this to the v1.0.0 milestone Sep 30, 2019
@0b10011
Copy link
Contributor Author

0b10011 commented Sep 30, 2019

@0b10011 Impressive work.

Thanks! I didn't quite realize how large the change was going to be until I was already well into it, but I'm pretty happy with how it turned out.

I'm just a bit worried about the non-backward compatibility of the changes.
This one will make it very difficult to migrate to a next version. It would be nice to be able to still pass sizes/colors/... the "old" way, and transform internally.

Agreed! Like I said, I didn't realize how large of a project was until I'd already done a lot of changes, so it isn't friendly to upgrading as-is.

Unfortunately, we'd lose the benefit of enforced types this way. I thought about manually checking types, but that would add a lot of boilerplate for the sole purpose of upgrading (which I know and believe is important to support somehow). I didn't do this for two reasons:

  1. The tests and code is already written, so it'd be a lot of work to go back in and add support for strings.
  2. This work would eventually be reverted, but it would have to be reverted by hand most likely if any meaningful amount of time passed.

What I think would work even better would be:

  1. Merge this PR with enforced types for the next version
  2. Merge a separate PR into the old/current version that coerces Length, Color, etc into the expected strings.

This will still be a lot of work, but then it will only need to be done once. That is, it won't need to be reverted later.

It's been a little bit since I submitted this, but I think there were some minor API changes along the way too. That should be able to be reflected in the current version as well.

Anyway, I'm more than happy to do this work as well. I just want to make sure the approach is good, the class signatures won't change, etc. I can even put together a proof of concept if you'd like to show what I mean. Just let me know!

@troosan troosan changed the title Check lengths, colors, styles, etc automatically Make methods strongly typed instead of using string input parameters Dec 15, 2019
@PowerKiKi
Copy link
Member

This looks like a super interesting thing PR. Would you be willing to rebase on develop ?

My plan would be to merge this, and release as a major version, and do nothing to help migration (!). The community might come up with a Rector rule for that...

@0b10011
Copy link
Contributor Author

0b10011 commented Sep 23, 2022

This looks like a super interesting thing PR. Would you be willing to rebase on develop ?

I unfortunately have covid right, and it's got me pretty worn down.

After I recover, it could be a fun project though! I'll set a reminder for myself in a week or so, and see if it's something I can take care of. (I'm no longer working on the project I was hoping to use this for, though, so no promises.)

I remember there being a bunch of other things that could use strict typing to greatly improve the library, too, but I can't remember exactly what. I'll try to note those things if I'm able to work on it.

@PowerKiKi
Copy link
Member

Nothing matter more than your health. Take all the time you need.

And if you find out that you're not so much interested in this anymore, that's totally fine too. Just let us know, so someone else might be picking it up later on...

@PowerKiKi PowerKiKi changed the base branch from develop to master November 16, 2022 21:11
@Progi1984 Progi1984 force-pushed the master branch 3 times, most recently from 2d9f999 to e458249 Compare August 30, 2023 11:56
@Progi1984 Progi1984 removed this from the 1.2.0 milestone Nov 24, 2023
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.

5 participants