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

Update to PHPWord 0.14.0 #11696

Merged
merged 2 commits into from
Feb 26, 2018
Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 20, 2018

PHPWord 0.14.0 was recently released:

https://github.com/PHPOffice/PHPWord/releases/tag/0.14.0

This fixes a number of bugs and adds some new features.

However, my only motivation for this update is that loosens some Zend dependencies in the composer.json that will allow CiviCRM to be required via composer on a Drupal 8 site.

I actually don't know where PHPWord is used in CiviCRM, and haven't tested that it actually works. I'm kind of hoping the automated tests will help with this. :-)

But increasing this dependency will allow removing the PHPWord fork from the Drupal 8 installation instructions.

@totten
Copy link
Member

totten commented Feb 20, 2018

Some suggestions on reviewing:

  • For r-run, do a search for contacts and choose the mail-merge/print-letter functionality. It has multiple output modes, including the original (PDF) and the newer (Word and OpenOffice).
  • For r-run, enable CiviCase. Try generating a letter there.
  • For r-user/r-tech, review release notes of PHPWord 0.14 to see if anything seems significant.

@eileenmcnaughton
Copy link
Contributor

I took a look at the release notes & there is nothing of concern there. fixes & features but nothing looks like it would cause breakage

@seamuslee001
Copy link
Contributor

why has composer.lock not been updated here?

@eileenmcnaughton
Copy link
Contributor

I tested & found it pretty confusing what you were supposed to do in case to use ms word but at it's simplest it seemed like you could do the same thing from case that you can from anywhere else & that seemed to still export to word / odt just fine.

@seamuslee001
Copy link
Contributor

@eileenmcnaughton can you retest after running composer update locally. My thinking is that it won’t have installed the updated version because composer.lock isn’t included in this PR

@eileenmcnaughton
Copy link
Contributor

Note that changes to composer.lock will fight with #11519 - I'm trying to get @totten to recheck & hopefully merge that one

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 26, 2018

Is there a process for updating composer.lock in the way that CiviCRM wants it updated? I tried doing composer update phpoffice/phpword --with-dependencies locally (because that's what I usually do), but, of course, I end up with some dependencies that require newer versions of PHP (since I'm using PHP 7.1 locally). So, I did composer update phpoffice/phpword instead which just updated phpword, which is what's in the last commit. Anyway, I'm happy to do the update, I just want to make sure I'm doing it the right way :-)

@eileenmcnaughton
Copy link
Contributor

I tested with the new composer lock file. I did not re-test civicase as it seemed no different to 'normal' mail merge. My feeling about this is that if there is to be a difference it would be on some obscure formatting we could not predict and testing is unlikely to find that. However, I do have a job to work on some Word formatting so I will apply this change to our site before doing that work & increase the chances of spotting anything

composer.lock looks OK to me. Merging

@eileenmcnaughton eileenmcnaughton merged commit cf5a942 into civicrm:master Feb 26, 2018
@eileenmcnaughton
Copy link
Contributor

I've been testing this and it fixes a problem with rendering line breaks. I thought at first the text align function was broken & I was seeing some other format details. However, after watching someone who actually has MS Word installed open the documents I could see that the problem was not in the actual document

@colemanw
Copy link
Member

@eileenmcnaughton so are you saying that upgrading a library in CiviCRM actually improved something? That's an exciting twist on what usually happens ;)

@eileenmcnaughton
Copy link
Contributor

@colemanw yep - it actually provided the fix to the problem I was trying to solve - although it took me a long time to realise that because I didn't realise the fix was not applied on the server I was testing from & also was seeing different results to our users because of my lack of MS word

@eileenmcnaughton
Copy link
Contributor

@lcdservices fyi

@mlutfy mlutfy added this to the 4.7.32 milestone Mar 6, 2018
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 20, 2018
This update improves handling of br tags & makes the address block
split over several lines as expected (without it the break does not show).

Note that my QA appeared to show a handful of problems but when I tested
with Leanne over screen share the problems were not present when actual 'MS
Word is used to open the docs rather than open office. I can share these screenshots
with a reviewer

civicrm#11696

Bug: T187251

Change-Id: I0a62ff74b41eedd26daed4eddeddf930ce939abd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants