read hyperlink for drawing image #490

tezrik · 2018-05-07T04:46:47Z

This is:

- [ ] a bugfix
- [X] a new feature

Checklist:

Changes are covered by unit tests
Code style is respected
Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
CHANGELOG.md contains a short summary of the change
Documentation is updated as necessary

Why this change is needed?

Read hyperlinks for drawing image

PowerKiKi

This looks like an interesting contribution, but it misses a few things to be merged. Could you take care of it ?

add write support for symetry
document feature in docs/references/features-cross-reference.md
cover with unit tests (see EnclosureTest example)

tezrik · 2018-05-07T17:00:56Z

sorry in my English
Ok, I'll try to do it in the near future.

tezrik · 2018-05-14T12:54:35Z

I think I had a problem. I can not understand the error. Please help me

criztovyl · 2018-05-14T13:15:23Z

The error is that some of the methods you changed no longer pass code quality checks.

criztovyl · 2018-05-14T13:18:37Z

src/PhpSpreadsheet/Writer/Xlsx/Drawing.php

@@ -179,6 +185,15 @@ public function writeDrawing(XMLWriter $objWriter, BaseDrawing $pDrawing, $pRela
            $objWriter->writeAttribute('id', $pRelationId);
            $objWriter->writeAttribute('name', $pDrawing->getName());
            $objWriter->writeAttribute('descr', $pDrawing->getDescription());
+
+            //a:hlinkClick
+            if ($pHlinkClickId >= 0) {


This if increases the "Condition count" from 6 to 7, which raised the rating from B to C.
The project's current configuration of Scrutinizer forbids ratings above or equal to C.

Scrutinizer about Drawing::writeDrawings()

Indeed, the if should be extract to new private method.

criztovyl · 2018-05-14T13:25:46Z

src/PhpSpreadsheet/Writer/Xlsx/Rels.php

+                        $drawing->getHyperlink()->getUrl(),
+                        $drawing->getHyperlink()->isInternal() ? '' : 'External'
+                    );
+                }
            }


The if and the ?: here raise the "Condition count" from 8 to 10 and together with the added lines themselves raised the rating from B to C.
Scrutinizer about Rels::writeDrawingRelationships().

Scrutinizer has some suggestions how to fix this particular issue.

Again the entire if should go into a new private method (even if it's short). Something like writeDrawingHyperLink(XMLWriter $objWriter, BaseDrawing $drawing)

criztovyl · 2018-05-14T13:36:29Z

If you are not sure how to fix the issues, wait for PowerKiki's comments about your further changes.

tezrik · 2018-05-15T07:17:28Z

Thank you for the clarification. I expect then a response from PowerKiki's

PowerKiKi

Thanks you for the modifications. The test and doc look good. There is a few things that can improve the code quality before merging. Please have a look.

PowerKiKi · 2018-05-20T09:19:23Z

samples/Reader/20_Reader_worksheet_hyperlink_image.php

+require __DIR__ . '/../Header.php';
+
+$inputFileType = 'Xlsx';
+$inputFileName = __DIR__ . '/sampleData/example3.xlsx';


We try to avoid adding XLSX files when possible to avoid growing the size of the repository. So please delete samples/Reader/sampleData/example3.xlsx, and either change the sample to write and read, or delete the sample entirely (but do not keep it with only read).

This has not been addressed.

PowerKiKi · 2018-05-20T09:21:07Z

src/PhpSpreadsheet/Reader/Xlsx.php

@@ -1606,6 +1613,14 @@ public function load($pFilename)
                                                        $shadow->getColor()->setRGB(self::getArrayItem($outerShdw->srgbClr->attributes(), 'val'));
                                                        $shadow->setAlpha(self::getArrayItem($outerShdw->srgbClr->alpha->attributes(), 'val') / 1000);
                                                    }
+                                                    if ($hlinkClick) {


This if must be extracted to a separate private method.

PowerKiKi · 2018-05-20T09:21:35Z

src/PhpSpreadsheet/Reader/Xlsx.php

@@ -1654,6 +1670,14 @@ public function load($pFilename)
                                                        $shadow->getColor()->setRGB(self::getArrayItem($outerShdw->srgbClr->attributes(), 'val'));
                                                        $shadow->setAlpha(self::getArrayItem($outerShdw->srgbClr->alpha->attributes(), 'val') / 1000);
                                                    }
+                                                    if ($hlinkClick) {


Idem, extract to method.

PowerKiKi · 2018-05-20T09:22:20Z

src/PhpSpreadsheet/Worksheet/BaseDrawing.php

+    /**
+     * Image hyperlink.
+     *
+     * @var Hyperlink


This should be @var null|Hyperlink

PowerKiKi · 2018-05-20T09:22:47Z

src/PhpSpreadsheet/Worksheet/BaseDrawing.php

@@ -508,4 +516,24 @@ public function __clone()
            }
        }
    }
+
+    /**
+     * @param Hyperlink $pHyperlink


This should be @var null|Hyperlink

PowerKiKi · 2018-05-20T09:25:08Z

src/PhpSpreadsheet/Worksheet/BaseDrawing.php

+    public function getHyperlink()
+    {
+        if ($this->hyperlink === null) {
+            $this->hyperlink = new Hyperlink();


Why do you create new Hyperlink if it does not exist, instead of returning null ?

PowerKiKi · 2018-05-20T09:27:49Z

src/PhpSpreadsheet/Writer/Xlsx/Drawing.php

@@ -179,6 +185,15 @@ public function writeDrawing(XMLWriter $objWriter, BaseDrawing $pDrawing, $pRela
            $objWriter->writeAttribute('id', $pRelationId);
            $objWriter->writeAttribute('name', $pDrawing->getName());
            $objWriter->writeAttribute('descr', $pDrawing->getDescription());
+
+            //a:hlinkClick
+            if ($pHlinkClickId >= 0) {


Indeed, the if should be extract to new private method.

PowerKiKi · 2018-05-20T09:28:57Z

src/PhpSpreadsheet/Writer/Xlsx/Drawing.php

     *
     * @throws WriterException
     */
-    public function writeDrawing(XMLWriter $objWriter, BaseDrawing $pDrawing, $pRelationId = -1)
+    public function writeDrawing(XMLWriter $objWriter, BaseDrawing $pDrawing, $pRelationId = -1, $pHlinkClickId = -1)


Let's use null instead of -1 for a better semantic of absence of value. Then we should also change @param null|int $pHlinkClickId.

PowerKiKi · 2018-05-20T09:36:08Z

src/PhpSpreadsheet/Writer/Xlsx/Rels.php

+                        $drawing->getHyperlink()->getUrl(),
+                        $drawing->getHyperlink()->isInternal() ? '' : 'External'
+                    );
+                }
            }


Again the entire if should go into a new private method (even if it's short). Something like writeDrawingHyperLink(XMLWriter $objWriter, BaseDrawing $drawing)

PowerKiKi · 2018-05-20T09:37:27Z

tests/PhpSpreadsheetTests/Functional/DrawingImageHyperlinkTest.php

+ * User: yuzhakov
+ * Date: 08.05.18
+ * Time: 12:00.
+ */


We do not use file comment blocks, please remove. You will be credited in the commit information.

tezrik · 2018-05-25T07:47:41Z

Ok, I'll try to do it in the near future.

tezrik · 2018-06-19T07:45:07Z

Sorry, but I can not understand how these tests are related to me. In my changes there are none.

PowerKiKi

There are still a few things to change in code.

I restarted the test job, let's see if it passes...

PowerKiKi · 2018-06-24T09:53:40Z

samples/Reader/20_Reader_worksheet_hyperlink_image.php

+require __DIR__ . '/../Header.php';
+
+$inputFileType = 'Xlsx';
+$inputFileName = __DIR__ . '/sampleData/example3.xlsx';


This has not been addressed.

PowerKiKi · 2018-06-24T09:54:08Z

docs/references/features-cross-reference.md

+        <td></td>
+        <td></td>
+        <td></td>
+        <td>$Drawing->getHyperlink()->getUrl($url)</td>


"$Drawing" should be "$drawing"

PowerKiKi · 2018-06-24T09:54:50Z

src/PhpSpreadsheet/Cell/Hyperlink.php

+     */
+    public function getTypeHyperlink()
+    {
+        return ($this->isInternal()) ? '' : 'External';


extra parenthesis should be removed

PowerKiKi · 2018-06-24T09:56:01Z

src/PhpSpreadsheet/Reader/Xlsx.php

+     * @param \SimpleXMLElement $cellAnchor
+     * @param array $hyperlinks
+     */
+    private function readHyperLinkDrawing(&$objDrawing, $cellAnchor, $hyperlinks)


Parameters must be type hinted. Also "&$objDrawing" must be "$objDrawing"

PowerKiKi · 2018-06-24T09:56:54Z

src/PhpSpreadsheet/Reader/Xlsx.php

+    private function readHyperLinkDrawing(&$objDrawing, $cellAnchor, $hyperlinks)
+    {
+        $hlinkClick = $cellAnchor->pic->nvPicPr->cNvPr->children('http://schemas.openxmlformats.org/drawingml/2006/main')->hlinkClick;
+        if ($hlinkClick) {


Inverse the condition to return early if there is no link

PowerKiKi · 2018-06-24T10:01:29Z

src/PhpSpreadsheet/Writer/Xlsx/Drawing.php

+     */
+    private function writeHyperLinkDriwing(XMLWriter &$objWriter, $pHlinkClickId)
+    {
+        if ($pHlinkClickId !== null) {


Inverse condition and return early if no link

PowerKiKi · 2018-06-24T10:02:05Z

src/PhpSpreadsheet/Writer/Xlsx/Rels.php

+     */
+    private function writeDrawingHyperLink($objWriter, $drawing, &$i)
+    {
+        if ($drawing->getHyperlink() !== null) {


Inverse condition and return early if no link

PowerKiKi · 2018-06-24T10:12:39Z

src/PhpSpreadsheet/Writer/Xlsx/Rels.php

+     *
+     * @throws WriterException
+     */
+    private function writeDrawingHyperLink($objWriter, $drawing, &$i)


Parameters should be typed and &$i should be avoided and instead use return value.

PowerKiKi · 2018-06-24T10:13:49Z

tests/PhpSpreadsheetTests/Functional/DrawingImageHyperlinkTest.php

+
+class DrawingImageHyperlinkTest extends AbstractFunctional
+{
+    const BASE_URL = 'https://github.com/PHPOffice/PhpSpreadsheet';


This should be a variable in the method to avoid public visibility.

PowerKiKi · 2018-06-24T10:15:13Z

tests/PhpSpreadsheetTests/Reader/XlsxTest.php

@@ -1,19 +0,0 @@
-<?php


This file and without_cell_reference.xlsx should probably not be deleted

tezrik · 2018-06-30T13:31:15Z

Hi, I ran tests on the local machine. All tests pass without errors.

PowerKiKi · 2018-07-15T04:06:08Z

Thanks ! I merged it.

Fixes PHPOffice#490

tezrik added 2 commits May 7, 2018 09:41

read hyperlink for drawing image

ba9eb09

code style fix

e85057b

PowerKiKi requested changes May 7, 2018

View reviewed changes

PowerKiKi added the missing test label May 7, 2018

tezrik added 5 commits May 8, 2018 13:03

add write support fore symetry

5c3515a

document feature in docs/references/features-cross-reference.md

7e95afa

cover with unit tests (see EnclosureTest example)

bb83901

Code style fix

d0c5313

remove die

2d2e61c

criztovyl reviewed May 14, 2018

View reviewed changes

PowerKiKi requested changes May 20, 2018

View reviewed changes

g.yuzhakov and others added 6 commits June 2, 2018 00:13

extract read hyperlink

98ead5b

bag fix

5befe13

code style fix

074018f

code style fix

a4e6b7c

Merge branch 'develop' into develop

25a2121

bag fix

ad89d0c

PowerKiKi requested changes Jun 24, 2018

View reviewed changes

g.yuzhakov added 4 commits June 25, 2018 14:57

Update code style and fix code

5bb7fb4

Revert: code style fix

91a5de9

fix test

2100a90

fix bag

35eb2fb

g.yuzhakov added 2 commits June 30, 2018 16:43

codestyle fix

886ce8e

fix

524db77

PowerKiKi closed this in 17d4a54 Jul 15, 2018

Dfred pushed a commit to Dfred/PhpSpreadsheet that referenced this pull request Nov 20, 2018

Read and write hyperlink for drawing image

5752f62

Fixes PHPOffice#490

read hyperlink for drawing image #490

read hyperlink for drawing image #490

Conversation

tezrik commented May 7, 2018 • edited Loading

Why this change is needed?

PowerKiKi left a comment • edited Loading

Choose a reason for hiding this comment

tezrik commented May 7, 2018 • edited Loading

tezrik commented May 14, 2018

criztovyl commented May 14, 2018

criztovyl May 14, 2018 • edited Loading

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

criztovyl commented May 14, 2018

tezrik commented May 15, 2018

PowerKiKi left a 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

tezrik commented May 25, 2018

tezrik commented Jun 19, 2018

PowerKiKi left a 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

tezrik commented Jun 30, 2018

PowerKiKi commented Jul 15, 2018

tezrik commented May 7, 2018 •

edited

Loading

PowerKiKi left a comment •

edited

Loading

tezrik commented May 7, 2018 •

edited

Loading

criztovyl May 14, 2018 •

edited

Loading