Two folders PHPWord and PhpWord in tests

[ivanlanin]

@Progi1984 @gabrielbull Please help. I found out that there's two folder with different capitalization in the tests folder that were created accidentally during the last merge. Since I'm using windows, I can't recognize the difference. Sorry to trouble you guys, but could you help to move all files from "tests/PHPWord" to "tests/PhpWord"? Thanks.

[gabrielbull]

Im on it right now

[gabrielbull]

put everything you do on hold, i'll push my changes in a few minutes

[ivanlanin]

Ok. Thanks.

[gabrielbull]

@RomanSyroeshko @ivanlanin @Progi1984
Ok, so I cleaned and refactored tests a bit. A few things:

Someone had rewrote the Autoloader, I had to re-write it back to make it PSR-4 compliant.

Someone had renamed the _files directory in tests to data, I changed that too.

Can we stop using this type of code to define paths:

$imageSrc = \join(
    \DIRECTORY_SEPARATOR,
    array(\PHPWORD_TESTS_BASE_DIR, 'data', 'images', 'PhpWord.png')
);

This is a lot easier to work with:

$src = __DIR__ . "/_files/images/PhpWord.png";

[ivanlanin]

Thanks. I changed _files into data because I look at phpDocumentor test folder structure and I thought it is good.

Ok, I'll change follow the easier path writing style :)

Thanks

[gabrielbull]

In PHPUnit documentation it is _files

[ivanlanin]

Ok. Sorry. I miss that reference. I'll run the tests again.

[gabrielbull]

Okay, also I removed CodeSniffer from travis because we are not passing it, either we put it in allowed failures or make sure we pass it before we add it back.

[ivanlanin]

Ok. I've removed most errors from phpcs. There are two sources of errors left, i.e. Shared\File::file_exists and all functions in Shared\ZipStreamWrapper. I've checked our codes and only file_exists used in two files. Since we're moving to 0.9, is it ok now to change the API?

[gabrielbull]

Sure, as along as it is in the new Doc and samples, that we pass tests, ect. it is fine.

[ivanlanin]

Ok

[ivanlanin]

I've reactivated phpcs for both src and test. We passed!

I don't know how to activate phpdoc test in Travis. I suggest we activate it too. I've checked and we've passed in my machine.

I'm off for now.

[ghost]

This is a lot easier to work with:
$src = DIR . "/_files/images/PhpWord.png";

@gabrielbull, this is easier and shorter in case of copy-paste. I wouldn't agree with you in case if we put directory names in constants, for example. It seems me more correct to have directory names defined somewhere once. What do you think about that? Maybe we should do that?

[gabrielbull]

We can absolutely create constants but I don't mind the relative paths so much.

[ghost]

Just to be on the same page. My concern is about refactoring paths. If paths are reresented by constants, they can be easily refactored. If paths are represented by copy-pasted strings, it is hard to refactor them, because you have to search and replace, to search and replace... Such approach takes more time and can cause some extra bugs. So, as for me, I vote for constants and for API which will construct paths via "join".

If you are agree, I will raise a new issue then.

[gabrielbull]

I'll think about it. Global constants are considered bad practice now a day, but I don't mind them in the unit tests. I'm really not sold as relative paths don't require refactoring, as they are relative to the directory they sit in. If we go the constants road, we should really limit them to a few, like PHPWORD_TESTS_BASE_DIR that points to /tests/PhpWord/Tests and thats it.

[ghost]

OK, just imagine that you have 50 paths like '_files/images/whatever.jpg'. They are all located in different classes. One day you decided to rename "images" folder into something different. If you are using some constant (global or class constant), all you need is only change its value. In case of string paths you need carefully replace 50 strings, right?

[gabrielbull]

Couldnt we use an helper class to access paths with static methods instead? Like this TestHelper::getImagesPath()

[ghost]

Yep, something like that, but I'm not sure about Helper. Helpers is a kind of procedural (not OO) programming, and there is no such design pattern as "Helper". I often hear that using helpers is a bad practice, because they violate OOP principles.

Several posts for consideration.

• http:https://blogs.msdn.com/b/nickmalik/archive/2005/09/06/461404.aspx
• http:https://blogs.msdn.com/b/elee/archive/2009/05/05/helper-classes-are-evil.aspx
• http:https://lostechies.com/chrismissal/2009/06/01/anti-patterns-and-worst-practices-utils-class/
• http:https://simpleprogrammer.com/2010/01/29/static-methods-will-shock-you/
• http:https://smart421.wordpress.com/2011/08/31/why-helper-singletons-and-utility-classes-are-mostly-bad-2/

On my mind, we deliver not only good functionality, but we show our users an example of how to design and write good code according to cutting-edge standards and principles. A developer will look at our implementation and make a decision that it's OK if I do this, it's OK if I do that... We are responsible for that. So, if your ask me, I would say that we should call that class in some other way, or maybe extend an existing one. My heart tells me that using Helpers it's a not a good idea, and we should avoid doing so.

If you are agree that it makes sense let's raise an issue and take some time to think about. I'm not ready to give you some design proposal right now. I would like just to indicate a problem and share my concerns.

Thanks.

[gabrielbull]

Okay, good points, lets sleep on that.

[ivanlanin]

On my mind, we deliver not only good functionality, but we show our users an example of how to design and write good code according to cutting-edge standards and principles.

I agree with this. Very much.