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

Composer implementation for dependencies and autoload #1912

Merged
merged 17 commits into from Jan 29, 2015

Conversation

Projects
None yet
4 participants
@Rarst
Contributor

Rarst commented Jan 16, 2015

Functional and ready to merge.

  • deprecates Git submodules
  • implements autoload via Composer w/ PHP 5.2 compatibility

See #1890

Intermediary instructions to install for testing (this will be down to single create-project in the end):

git clone https://github.com/Rarst/wordpress-seo --branch composer
cd wordpress-seo
composer install --no-dev

@Rarst Rarst referenced this pull request Jan 16, 2015

Closed

Composer roadmap #1890

10 of 10 tasks complete
@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 16, 2015

Contributor

Interesting aspect about testing — can you build using PHP 5.3+ on Travis, but test using PHP 5.2?

Contributor

Rarst commented Jan 16, 2015

Interesting aspect about testing — can you build using PHP 5.3+ on Travis, but test using PHP 5.2?

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 19, 2015

Contributor

Tests working, except coding style, see https://travis-ci.org/Yoast/wordpress-seo/jobs/47506113

Outstanding issues with that:

  1. PHP 5.2 linter cannot parse some of vendor (not used, but still present).
  2. Similarly WPCS detects errors with some of vendor.

Exclude vendor from checks altogether? Everything except vendor/yoast ?

Contributor

Rarst commented Jan 19, 2015

Tests working, except coding style, see https://travis-ci.org/Yoast/wordpress-seo/jobs/47506113

Outstanding issues with that:

  1. PHP 5.2 linter cannot parse some of vendor (not used, but still present).
  2. Similarly WPCS detects errors with some of vendor.

Exclude vendor from checks altogether? Everything except vendor/yoast ?

@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 19, 2015

Member

I'd exclude vendor indeed.

Member

jdevalk commented Jan 19, 2015

I'd exclude vendor indeed.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 20, 2015

Contributor

Oookay, Composer all working and tests all green, including coding style and PHP 5.2

Next — writing up docs (where is good place for it?), getting team up to speed, and updating build process (which is what?).

It would be helpful if team started to try out intermediary install instructions above and enumerated what doesn't work smoothly and/or needs explaining.

Contributor

Rarst commented Jan 20, 2015

Oookay, Composer all working and tests all green, including coding style and PHP 5.2

Next — writing up docs (where is good place for it?), getting team up to speed, and updating build process (which is what?).

It would be helpful if team started to try out intermediary install instructions above and enumerated what doesn't work smoothly and/or needs explaining.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 20, 2015

Contributor

The wiki seems like a logical place for instructions and such and possibly for the documentation as well ?

Contributor

jrfnl commented Jan 20, 2015

The wiki seems like a logical place for instructions and such and possibly for the documentation as well ?

@@ -2791,192 +2791,195 @@ protected function clean_option( $option_value, $current_version = null, $all_ol
/*******************************************************************
* Option: wpseo_ms
*******************************************************************/
if ( is_multisite() ) {

This comment has been minimized.

@jrfnl

jrfnl Jan 20, 2015

Contributor

If this conditional is removed, the WPSEO_Options constructor will need to be adjusted as it now checks for whether the class exists before creating/loading that option which would no longer be a valid test once you remove this.

@jrfnl

jrfnl Jan 20, 2015

Contributor

If this conditional is removed, the WPSEO_Options constructor will need to be adjusted as it now checks for whether the class exists before creating/loading that option which would no longer be a valid test once you remove this.

This comment has been minimized.

@Rarst

Rarst Jan 20, 2015

Contributor

I've moved the check to the _MS class' constructor (after checking in w/ @jdevalk if that makes sense in architecture). It will be instanced, but won't run logic unless in multisite context.

@Rarst

Rarst Jan 20, 2015

Contributor

I've moved the check to the _MS class' constructor (after checking in w/ @jdevalk if that makes sense in architecture). It will be instanced, but won't run logic unless in multisite context.

This comment has been minimized.

@jrfnl

jrfnl Jan 20, 2015

Contributor

@Rarst I'd have to look in detail, but I suspect that will still make some things go wrong as in the WPSEO_Options class, the available instances are used for certain actions and that logic relies on the _ms option not being instantiated (when not on multisite).

@jrfnl

jrfnl Jan 20, 2015

Contributor

@Rarst I'd have to look in detail, but I suspect that will still make some things go wrong as in the WPSEO_Options class, the available instances are used for certain actions and that logic relies on the _ms option not being instantiated (when not on multisite).

This comment has been minimized.

@jdevalk

jdevalk Jan 21, 2015

Member

Hmm ok, @Rarst can you look at that specific bit? :)

@jdevalk

jdevalk Jan 21, 2015

Member

Hmm ok, @Rarst can you look at that specific bit? :)

This comment has been minimized.

@Rarst

Rarst Jan 21, 2015

Contributor

I stared at it and need a little more specific pointer towards part that might be going wrong. :)

@Rarst

Rarst Jan 21, 2015

Contributor

I stared at it and need a little more specific pointer towards part that might be going wrong. :)

This comment has been minimized.

@jrfnl

jrfnl Jan 21, 2015

Contributor

I don't have the time at the moment to look at it and argue it in detail. The short of it is that your are changing the expected behaviour of the object without cause.

I suggest you either change the conditional in the WPSEO_Options class as I suggested before or add the conditional to the get_instance() method of the _ms class instead of the constructor. That way the behaviour doesn't change (instance will be null rather than a nearly empty object).
The constructor of the WPSEO_Options class needs to change either way as the whole class_exists() doesn't make sense anymore with all option classes always being available.

@jrfnl

jrfnl Jan 21, 2015

Contributor

I don't have the time at the moment to look at it and argue it in detail. The short of it is that your are changing the expected behaviour of the object without cause.

I suggest you either change the conditional in the WPSEO_Options class as I suggested before or add the conditional to the get_instance() method of the _ms class instead of the constructor. That way the behaviour doesn't change (instance will be null rather than a nearly empty object).
The constructor of the WPSEO_Options class needs to change either way as the whole class_exists() doesn't make sense anymore with all option classes always being available.

This comment has been minimized.

@Rarst

Rarst Jan 21, 2015

Contributor

Added skip for MS only instances in Options constructor in 0decfde

@Rarst

Rarst Jan 21, 2015

Contributor

Added skip for MS only instances in Options constructor in 0decfde

This comment has been minimized.

@jrfnl

jrfnl Jan 21, 2015

Contributor

@Rarst Thanks. Oh... the class_exists() can go now and the else should also apply to the code you've now added.

@jrfnl

jrfnl Jan 21, 2015

Contributor

@Rarst Thanks. Oh... the class_exists() can go now and the else should also apply to the code you've now added.

This comment has been minimized.

@Rarst

Rarst Jan 21, 2015

Contributor

I misunderstood else (thought it's cleaning up instances, not pruning input list). Adjusted in https://github.com/Rarst/wordpress-seo/commit/797d6039205b062cfd30efeb7b422be7b45333a2

@Rarst

Rarst Jan 21, 2015

Contributor

I misunderstood else (thought it's cleaning up instances, not pruning input list). Adjusted in https://github.com/Rarst/wordpress-seo/commit/797d6039205b062cfd30efeb7b422be7b45333a2

This comment has been minimized.

@jrfnl

jrfnl Jan 21, 2015

Contributor

No worries, looks all good now ;-) Thanks for bearing with me on this.

@jrfnl

jrfnl Jan 21, 2015

Contributor

No worries, looks all good now ;-) Thanks for bearing with me on this.

@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 21, 2015

Member

I agree with @jrfnl on the wiki being the spot. @omarreiss could you see whether you can get it to work already based on the input by @Rarst in this pull?

Member

jdevalk commented Jan 21, 2015

I agree with @jrfnl on the wiki being the spot. @omarreiss could you see whether you can get it to work already based on the input by @Rarst in this pull?

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 22, 2015

Contributor

I think this is now in good shape to merge into trunk and start testing out in that context (so that going through my fork step isn't required).

Although I would hold on that if anyone has unresolved implementation and/or workflow questions?

Contributor

Rarst commented Jan 22, 2015

I think this is now in good shape to merge into trunk and start testing out in that context (so that going through my fork step isn't required).

Although I would hold on that if anyone has unresolved implementation and/or workflow questions?

@jdevalk

View changes

Show outdated Hide outdated composer.json
@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 22, 2015

Member

One last minor comment and then I'm all for it. @omarreiss any feedback?

Member

jdevalk commented Jan 22, 2015

One last minor comment and then I'm all for it. @omarreiss any feedback?

@omarreiss

This comment has been minimized.

Show comment
Hide comment
@omarreiss

omarreiss Jan 22, 2015

Member

Nothing much, maybe drop a line in the Readme about local (development) setup? What steps do I need to take to get the plugin running from source?

Member

omarreiss commented Jan 22, 2015

Nothing much, maybe drop a line in the Readme about local (development) setup? What steps do I need to take to get the plugin running from source?

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 22, 2015

Contributor

I'll update readme in a bit (need to eat :) ). The draft of instructions for common procedures (I could think of right away) is already up in in the wiki, ask if anything.

Contributor

Rarst commented Jan 22, 2015

I'll update readme in a bit (need to eat :) ). The draft of instructions for common procedures (I could think of right away) is already up in in the wiki, ask if anything.

@omarreiss

This comment has been minimized.

Show comment
Hide comment
@omarreiss

omarreiss Jan 22, 2015

Member

@Rarst Nice Wiki article! I had somehow missed that. I guess it would be more than enough to drop a reference to that article in the readme.

Member

omarreiss commented Jan 22, 2015

@Rarst Nice Wiki article! I had somehow missed that. I guess it would be more than enough to drop a reference to that article in the readme.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 26, 2015

Contributor

Readme updated, workflow possibilities ironed out some in the issue.

Contributor

Rarst commented Jan 26, 2015

Readme updated, workflow possibilities ironed out some in the issue.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 29, 2015

Contributor

Anything lingering that needs to happen for merge? :)

Contributor

Rarst commented Jan 29, 2015

Anything lingering that needs to happen for merge? :)

@omarreiss

This comment has been minimized.

Show comment
Hide comment
@omarreiss

omarreiss Jan 29, 2015

Member

👍 from me. @jrfnl @jdevalk ?

Member

omarreiss commented Jan 29, 2015

👍 from me. @jrfnl @jdevalk ?

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 29, 2015

Contributor

I don't think I'll have time to play with this (test it) before I'll be travelling again, so go for it.
I'll try and get it working when I'm back in March and if I run into trouble with it (which I no doubt will, through no fault but my own), I'll contact @Rarst.

I just won't be able to run any more tests or send in PRs between when this is merged and when I'll have the time to get things working in two months time.

Contributor

jrfnl commented Jan 29, 2015

I don't think I'll have time to play with this (test it) before I'll be travelling again, so go for it.
I'll try and get it working when I'm back in March and if I run into trouble with it (which I no doubt will, through no fault but my own), I'll contact @Rarst.

I just won't be able to run any more tests or send in PRs between when this is merged and when I'll have the time to get things working in two months time.

jdevalk added a commit that referenced this pull request Jan 29, 2015

Merge pull request #1912 from Rarst/composer
Composer implementation for dependencies and autoload

@jdevalk jdevalk merged commit 3686346 into Yoast:trunk Jan 29, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 29, 2015

Member

Merged :D

Member

jdevalk commented Jan 29, 2015

Merged :D

@Rarst Rarst deleted the Rarst:composer branch Jan 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment