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

Source code migration to PHP 5.3 features #58

Closed
2 tasks done
ghost opened this issue Jan 17, 2014 · 92 comments
Closed
2 tasks done

Source code migration to PHP 5.3 features #58

ghost opened this issue Jan 17, 2014 · 92 comments
Milestone

Comments

@ghost
Copy link

ghost commented Jan 17, 2014

As soon as minimal supported PHP version of PHPWord is 5.3 for now, it would be really cool to refactor source code and use new features (namespaces, for example).

Migration guide from PHP 5.2.x to PHP 5.3.x could be found here. Tasks to do:

@Progi1984, @gavroche, do we plan to do something like that?

@Progi1984
Copy link
Member

Could you open an issue to PHPExcel ?

It's the reference for PHPOffice projects.

Link : #40 (comment)

@ghost
Copy link
Author

ghost commented Jan 20, 2014

Sure, but there is small problem with that. PHPExcel still currently supports PHP 5.2. :)

It seems the topic is not actual for them.

@Progi1984
Copy link
Member

Yep but the version 1.7.9/1.8.0 is the last one to support this.

The next version 2.0.0 will support from 5.3.0.
Reference : PHPOffice/PHPExcel#183

@ghost
Copy link
Author

ghost commented Feb 15, 2014

There is still no answer from PHPExcel team.

@Progi1984, I suggest starting to support PHP 5.3 features independently of the PHPExcel project. php.net announced 5.6 alpha version not so long ago. We can't wait anymore. We're losing time and opportunities. It's time to discuss PHP 5.5 support, but we are stuck with 5.3 still. :(

What do you think?

@gabrielbull
Copy link
Member

+1 for not waiting for PHPExcel. If both projects agree to fellow the PHP-FIG standards instead of the PHPExcel standard they would get more support from the community and they could move faster.

@Progi1984 Progi1984 mentioned this issue Mar 3, 2014
12 tasks
@Progi1984
Copy link
Member

@RomanSyroeshko || @gabrielbull : Would you take care of this issue (that I link to #77) ?

@gabrielbull
Copy link
Member

Can you merge #76 and #78 in develop before?

@Progi1984
Copy link
Member

@gabrielbull Could you point these pull requests to the develop branch, before ?

@ghost
Copy link
Author

ghost commented Mar 6, 2014

Guys, do we have some exact dates for our milestones (0.7.1, 0.7.2, etc.)? I have already made some commitments, but I can't promise to do more, because I have no even preliminary schedule. :(

@Progi1984
Copy link
Member

@RomanSyroeshko :
Milestones are composed issues. Actually, in the 0.8.0 (https://github.com/PHPOffice/PHPWord/issues?direction=desc&milestone=2&page=1&sort=updated&state=open), we are at 48%.

New tickets are reported to next milestones, except simple questions.

@ghost
Copy link
Author

ghost commented Mar 6, 2014

So, we plan to release each version just when it ready (no time expectations), right?

@Progi1984
Copy link
Member

Affirmative. It's why i try to not load each milestone.

@ghost
Copy link
Author

ghost commented Mar 7, 2014

OK. I'll take care of this issue, if @gabrielbull leaves something for me to the moment when I handle my earlier commitments. :)

@Progi1984
Copy link
Member

@RomanSyroeshko Do you plan to do in this milestone or in this next ? Thx for your engagement ;)

@ghost
Copy link
Author

ghost commented Mar 7, 2014

I prefer in next, but will see how it goes. I saw that @gabrielbull has implemented something up on this subject.

@Progi1984 Progi1984 added this to the 0.7.2 milestone Mar 9, 2014
@Progi1984
Copy link
Member

I just assign it to the milestone 0.7.2.

And @MarkBaker has contacted me for the refactoring :

If it helps anybody with PHPWord refactoring for PHP 5.3, I've attached some of my experimental code for PHPExcel 2.

I'd been looking at moving common components (Autoloader, OLE, Document properties, zip handling, etc) across all apps into a separate PHPOffice repo and using a bootstrap into the application that would allow PHPWord, PHPExcel, etc to share a common autoloader (with separate namespaces) making it easier to work with more than one application within the PHPOffice set.
I'd modified the autoloader to work with a factory as well, so new objets are always instantiated through the factory making it easier for unit testing, but also easier for userland extensions to core classes.

I'm not completely happy with this yet, which is why it hadn't been posted anywhere; but with the postings yesterday about the revival of PHPWord development, I thought I'd throw my 2c into the pot now

You can download the file there (https://drive.google.com/file/d/0B8Md-dZwYjloNFhiQnN1SGZqY0c5djdEYmM4a05yQW5CSl9F/edit?usp=sharing)

And you can contact @MarkBaker if you need.

@Progi1984
Copy link
Member

@gabrielbull Your two pull requests are merged :)

@gabrielbull gabrielbull mentioned this issue Mar 10, 2014
@ghost
Copy link
Author

ghost commented Mar 10, 2014

Guys, I went through the migration guide and came to a conclusion that in scope of this issue we have to complete only two tasks (see issue description). So, the problem is not so huge as it was. :)

@ivanlanin
Copy link
Contributor

I agree. Let's migrate :)

@Progi1984
Copy link
Member

Is the fact of change in PHP 5.3 could not be the opportunity to evolve the core as implied @MarkBaker?

@ghost
Copy link
Author

ghost commented Mar 11, 2014

Of cource it's a good moment to get maximum from the changes provided by @MarkBaker, but lets consider that as the next step. I would say lets consider that as the separate issue in scope of 0.7.2.

@Progi1984
Copy link
Member

@RomanSyroeshko Okay, I let you create the separate issue for gathering ideas.

@gabrielbull
Copy link
Member

Me. It is standard to have the source files in src/ProjectName as there are multiple instances where you could have more than one project in the src directory.

Here are a few popular projects if you want to get some inspirations on PHP projects structures:

@ghost ghost mentioned this issue Mar 25, 2014
@ghost
Copy link
Author

ghost commented Mar 25, 2014

@gabrielbull, all of them use PSR-0 autoloader (take a look at composer.json). We use PSR-4 for autoloading in our composer.json. Main goal of PSR-4 is in removing namespace prefix and in shortening paths.

If we use long paths, there is no reason of using PSR-4.

@gabrielbull
Copy link
Member

PSR-0 is part of PSR-4 so we are compliant with PSR-4. The reason for PSR-4 is not necessarily for small libraries like this one but for much larger projects that have many parts and needs more accommodations like frameworks or applications. I think the structure we currently have is an accepted standard and I'm fine with it.

@ghost
Copy link
Author

ghost commented Mar 25, 2014

@ivanlanin, what do you think about that?

@ivanlanin
Copy link
Contributor

If it still complies with PSR, I'll go with the shortest path. I can imagine a new user checking out PhpWord. It will be better if he/she can find our files faster.

@ghost
Copy link
Author

ghost commented Mar 25, 2014

If it still complies with PSR, I'll go with the shortest path. I can imagine a new user checking out PhpWord. It will be better if he/she can find our files faster.

Yep. If a user downloads our sources packed in a ZIP-file, he already has PhpWord folder (or will create). In that case he will get PhpWord\src\PhpWord..., which is confusing. If a user downloads our sources via Composer, he will get phpoffice/phpword/src/PhpWord/..., which is confusing too. Really, don't see any necessity in additional and useless folder. People invented PSR-4 to help developers using shortest paths and we're going to ignore that. That's not cool. :(

@gabrielbull
Copy link
Member

Well, it's not confusing, it's a standard, meaning it is less confusing for people used to code PHP. Even the composer package uses this standard structure: https://github.com/composer/composer.

If everyone starts inventing new project structures because with PSR-4 you can, that is not better for the community, that is worse. See, we could have all our source files into a directory name /dir and that would have no meaning whatsoever and still be PSR-4 compliant.

@ghost ghost mentioned this issue Mar 25, 2014
@ghost
Copy link
Author

ghost commented Mar 25, 2014

@gabrielbull, if everyone does the same what others do, we will always stay on the same place. Here is another post about PSR-4 and PSR-0: Battle of the Autoloaders: PSR-0 vs. PSR-4.

So, if we use PSR-0 directory structure, we must use PSR-0 autoloading and do not confuse users with PSR-4 "compliance", because it's not true. If we claim that our code compliant with PSR-4, we must use appropriate directory structure.

@Progi1984, what's your opinion on the topic?

@Progi1984
Copy link
Member

Personnally, when I want a library, I use composer so I don't look at the architecture of the library.
Else, if it is not with composer, i put all in a directory and i just made one 'require' so I don't look at the architecture, too.

Then, if we support PSR-0 & PSR-4, i am agree with @gabrielbull.

@ghost
Copy link
Author

ghost commented Mar 25, 2014

@Progi1984, we can't support both. PSR-4 is designed to shorten paths. If we use long paths, it's not PSR-4, it's PSR-0.

Personnally, when I want a library, I use composer so I don't look at the architecture of the library.
Else, if it is not with composer, i put all in a directory and i just made one 'require' so I don't look at the architecture, too.

And what happens when you want to modify that library? You will have to look at the architecture.

@gabrielbull
Copy link
Member

I fail to see where PSR-0 is not compliant with PSR-4. Can you give us the points we are not complying with?

@ghost
Copy link
Author

ghost commented Mar 25, 2014

I fail to see where PSR-0 is not compliant with PSR-4.

Did I say so?

@gabrielbull
Copy link
Member

we must use PSR-0 autoloading and do not confuse users with PSR-4 "compliance"

Then, personally, I fail to see what is the rush for changing the project structure if it is compliant with all PSR-* and is using the standard in the industry.

@ghost
Copy link
Author

ghost commented Mar 25, 2014

According to PSR-4 the structure should like src/ instead of src/PhpWord/... or even src/PhpOffice/PhpWord/..., no?

@gabrielbull
Copy link
Member

@ghost
Copy link
Author

ghost commented Mar 25, 2014

Rows #1 and #2 in the table there. This is exactly what I'm trying to say.

@gabrielbull
Copy link
Member

But those are examples and do not include or exclude every possible scenarios, you have to read the specification and we must comply with every point there to be PSR-4 compliant, the bonus is that we are also PSR-0 compliant!

@ghost
Copy link
Author

ghost commented Mar 25, 2014

Are you sure about Composer? Does it support both?

@gabrielbull
Copy link
Member

Composer supports both.

@ghost
Copy link
Author

ghost commented Mar 25, 2014

OK, let it be so. That means I understand PSR-0 and PSR-4 wrong, as Jordi Boggiano does. I'm comfortable with that. Nevermind.

@gabrielbull
Copy link
Member

Well here is what I think. The current structure is fine for a release, we should not hold up the release because we are arguing over new standards that are not yet widely used.

What we can do, is keep an open mind to this and continue to talk about it. I've talked to Jordi in the past, I will talk to him again about this, even though he is very busy.

So what do you say we release the new version as is, and continue the discussion on this topic.

If you agree, close this issue and lets open a new email discussion about it very soon.

@ghost ghost mentioned this issue Mar 25, 2014
@ghost
Copy link
Author

ghost commented Mar 25, 2014

Guys, I've finished with cutting \PhpOffice\PhpWord\Shared\File class out. You may release the version, if you want.

I will take several days off and will be passive. Need some rest.

@ghost ghost closed this as completed Mar 25, 2014
@Progi1984
Copy link
Member

Rest you :) You have done a very good job :)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants