#160 Refactor folders: Element, Container, and Exception

[ivanlanin]

Part of #160:

• Rename folder Section to Element
• Rename folder Exceptions to Exception
• Flatten the Element folder structure (move PreserveText, Cell, and Row up one level)
• Create Container folder and move Section, Header, Footer, Settings into Container
• Create Container abstract class, inherited by Section, Header, Footer, Footnote, TextRun, and Cell
• Allow some elements in containers (e.g. link and list in header/footer, image in footnote)
• Deprecate createSection, createHeader, createFooter, createFootnote, and createTextRun in favor of uniform add... methods.
• Create Element abstract class

[ivanlanin]

@Progi1984 @gabrielbull @RomanSyroeshko I've finished refactoring based on #160 plan. Please review.

This refactoring should be backward compatible, but since there are a lot of changes in the code structure, I suggest we launch this as a new minor version (0.10) instead of 0.9.2.

Thanks

[gabrielbull]

Nice, I like that you started using the builder design pattern.

[ghost]

@Progi1984 @gabrielbull @RomanSyroeshko I've finished refactoring based on #160 plan. Please review.

Please, let me time until Monday. Lack of free time. :(

I suggest we launch this as a new minor version (0.10) instead of 0.9.2.

Good idea. I'm with you.

[ivanlanin]

@gabrielbull Thanks for the link. So that's the name for the method :) I'll learn more from the link that you gave. I hope this approach will reduce code duplication and simplify the process of adding more elements.

@RomanSyroeshko Sure. No problem. I'll be adding more refactoring, especially for Media, Header, and Footer. But all changes will still be backward compatible.

[Progi1984]

@ivanlanin Very good job. A very good refactoring which permits to work on a better base.
And with PHP-CPD, are we close to 0 error ?

[ivanlanin]

@Progi1984 Thanks. Yes, PHP-CPD reports better result, but unfortunately, there are still some exact clones. My aim is to eliminate all duplicates. I'll continue pursuing that goal while maintaing backward compatibility.

[Progi1984]

@ivanlanin Which clones are again there ?

[ivanlanin]

Here's from my latest branch:
phpcpd 2.0.0 by Sebastian Bergmann.

Found 6 exact clones with 508 duplicated lines in 9 files:

D:\www\local\phpword\src\PhpWord\Element\CheckBox.php:73-138
D:\www\local\phpword\src\PhpWord\Element\Text.php:62-127

D:\www\local\phpword\src\PhpWord\Container\Settings.php:372-567
D:\www\local\phpword\src\PhpWord\Style\Cell.php:216-411

D:\www\local\phpword\src\PhpWord\Container\Settings.php:428-589
D:\www\local\phpword\src\PhpWord\Style\Table.php:300-461

D:\www\local\phpword\src\PhpWord\Writer\ODText\Content.php:51-72
D:\www\local\phpword\src\PhpWord\Writer\ODText\Styles.php:39-60

D:\www\local\phpword\src\PhpWord\Writer\ODText\Content.php:122-154
D:\www\local\phpword\src\PhpWord\Writer\ODText\Styles.php:69-101

D:\www\local\phpword\src\PhpWord\Writer\ODText.php:42-76
D:\www\local\phpword\src\PhpWord\Writer\Word2007.php:68-102

2.53% duplicated lines out of 20044 total lines of code.

[ghost]

Hi, @ivanlanin, one question: why did you name the class as Container? Why Container?

[ivanlanin]

Because those classes (section, header, footer, textrun, footnote, and cell) share the same characteristic that they all can contain other elements (text, etc). Other options that I can offer are "stories" (from OOXML spec) and "destinations" (from RTF spec). OpenDocument spec doesn't give me any specific name.

Other "containers" that we can create later are, e.g., endnote, comment, and textbox.

[ghost]

According to ISO/IEC 29500-1 (known as "OOXML"):

• Table Cell (see the chapter 17.4.65) is a regular element and should reside in Element folder in our sources.
• Text Run (see the chapter 17.3.2.25) is a regular element and should reside in Element folder in our sources.
• Those Header, Footer, Footnotes you are talking about are regular elements too. There may be so called package parts (see the chapter 16.1) and there you could find Footer and Header too, but that's a different story.

P.S.
I suppose, that RTF, OOXML, ODF packages use different terminology, structure and based on different standards, so we will have to split the code into sub-modules in the nearest future.

[ivanlanin]

Thanks for the link. My references were from ECMA and are indeed older than the ISO version.

I agree that what I called Container is Element too. I implement that by inherit Container from Element. I create the Container class to create template methods (addText, addTextRun, etc) so that we don't have to duplicate the code.

I agree that different doc format will have different terminology. Since we are aiming to "bridge" those formats, we will have to define our own shared features and elaborate specific features for individual doc formats.

So, this is what I think we should decide:

• What will be the class name of the general element class? I suppose we can agree on Element.
• What will be the class name of the elements that can contain other elements and share add... methods?

[ghost]

Since we are aiming to "bridge" those formats...

What do you mean?

What will be the class name of the general element class?

AbstractElement

What will be the class name of the elements that can contain other elements and share add... methods?

Not sure that I understant what the problem is. Could you please clarify?

[ivanlanin]

1. Different specs may have different terminology while they are actually the same thing. One example is line-break in ODT which is actually a textbreak. Since PHPWord started with OOXML, we use a lot of terminology from OOXML
2. Shall we use Abstract for prefix for all other abstract classes, e.g. Writer and WriterPart?
3. The addText, addImage, and other methods are shared between section, header, footer, textrun, footnote, and cell. That's why I create Container abstract class which is inherited from Element. If we don't create the Container class, where should we put those common methods? In Element abstract class?

[ghost]

Different specs may have different terminology while they are actually the same thing. One example is line-break in ODT which is actually a textbreak. Since PHPWord started with OOXML, we use a lot of terminology from OOXML

As for me, the last thing I want is to invent our own terminilogy and standards here. The more file formats we support, the harder the code will be. It will be a real pain to deal with it. Moreover, our users will have to learn and understand that new general standard instead of just working with lib. It will be a lot simplier to use modular structure where each module supports one file format. Sure, common things should be in common place. This aproach let us follow native standards for each file format and use the appropriate terminilogy. Why somebody have to dive into some new terminilogy and phylosophy when he just needs to write/read OOXML? It's a good way to lose the audience.

Shall we use Abstract for prefix for all other abstract classes, e.g. Writer and WriterPart?

Sure.

• Zend Framework uses such prefix.
• PHP-FIG told us to do so too.

If we don't create the Container class, where should we put those common methods? In Element abstract class?

Why not?

[ivanlanin]

Ok. Based on the discussion, this is what I'll do:

• Stick with OOXML terminology as much as possible since this is our base platform.
• Remove Container and put the contents to Element.
• Give Abstract prefix for all abstract classes and Interface suffix for all interfaces (as recommended by PHP-FIG).

Is there anything else that we should consider?

[ghost]

Is there anything else that we should consider?

Who knows? Will let you know if something comes to my mind. :)

[ivanlanin]

I'm finished :) Kindly check for the last time since I want to move on to enhance the Reader.

All changes should still be backward compatible, but I suggest we release this as 0.10.0. Thanks.

[ghost]

Why such a hurry? Maybe we will wait for month or two before the next major release?

[ivanlanin]

I didn't mean we should release now :) I mean "when we release", we should release as 0.10.0 instead of 0.9.2.

I close this pull now. Thanks.

[ghost]

@ivanlanin, couple questions.

1. Why do we need such extremely short comments like that?

/**
 * Section
 */
class Section extends AbstractElement
{

I suggest killing them or replacing with something more meaninful. How do you think?

2. I see that there are some unqualified class names in DockBlocks. Here is what phpDocumentor's help tells about that.

A valid class name seen from the context where this type is mentioned. Thus this may be either a Fully Qualified Class Name (FQCN) or if present in a namespace a local name.

As for me, that means the two things.

• If I'm going make a reference to imported PhpOffice\PhpWord\Exception\Exception class from a DockBlock somewhere in PhpWord class, I should put @throws PhpOffice\PhpWord\Exception\Exception instead of @throws Exception.
• If I'm going make a reference to Template class from a DockBlock somewhere in PhpWord class, I should put @param Template, because the referenced class is located in the same namespace as PhpWord.

Sure, class names in phpDoc annotations can be resolved using usestatements, but there are some exceptions (phpUnit annotations, for example). So, to avoid of confusing situations I suggest following the rules and using unqualified class names in DockBlocks only in case of namespace match. For all other cases I suggest using fully qualified class names in DockBlocks.

@gabrielbull, what's your opinion on p. 2?

[ivanlanin]

1. It's just quick fixes for phpDoc compliance. There were 400+ phpDoc errors when I start working with docblock and now there are none. Yes, the very short comment are "cheats" :) I prefer (and plan to do) the second choice: put something longer and more meaningful there. Please don't remove it for now.
2. phpDoc behaves the same way as PHP compiler does. They don't put any extra rules. As long as PHP compiler understand the class declaration, phpDoc will understand it too. I don't think there's a need to make it longer as required.

[ghost]

1. OK
2. PHPUnit annotations such as @covers do not support unqualified class names.

[ivanlanin]

Ok. I agree with the rule that we only use unqualified names in docblocks for classes in the same namespace.