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

Allow usage with v2 or v3 ZF components #38

Merged
merged 11 commits into from
Aug 17, 2016

Conversation

weierophinney
Copy link
Collaborator

This patch modernizes the module to allow it to work with either v2 or v3 components. The changes include:

  • Removing the root Module.php file. If folks are not using Composer for their autoloading, they should; if they cannot, they can setup autoloading manually.
  • Bump the minimum supported PHP version to 5.6, just as was done with all components on which this depends.
  • Bump the phpcs version to 2.6.2, and setup a config file to allow automating things like short-array checks/conversion.
  • Setup a modern Travis CI configuration including:
    • docker-based builds (much, much faster!)
    • caching of composer artifacts
    • lowest/locked/latest test strategy, to ensure changes are both backwards compatible with the minimum supported versions, and forwards compatible with the latest.
    • usage of composer scripts, so that changes in QA tools will not affect the build workflow
  • Updates to tests to ensure that they work with 5.X versions of PHPUnit.

Locally, everything passes just fine against both v2 and v3 components, but we'll see what Travis says.

- Removed root package `Module.php` file; it's essentially unnecessary, and
  folks can add autoloading to their application manually if not using Composer.
- Use short array notation, and split multiple entries over multiple lines in
  the test bootstrap.
- Added phpcs configuration, and bumped minimum required version to 2.6.2
- Set up composer scripts for QA tools.
- Set up a modern test matrix for Travis CI.
  - lowest/locked/latest testing strategy, to pick up when a change may not be
    backwards and/or forwards compatible
  - use docker builds
  - cache dependencies between builds
  - reference composer scripts, so that the build configuration may not
    necessarily need to change if QA tools change
- Bump dependencies to versions known to be stable and forwards compatible with
  v3 releases.
- For forwards compatibility with PHPUnit 5.
- Adds `Zend\Router` as a module to the test configuration
- Calls `disableOriginalConstructor()` on select mock instances where
  needed
@coveralls
Copy link

coveralls commented Aug 16, 2016

Coverage Status

Coverage remained the same at 96.992% when pulling a9bbdf9 on weierophinney:feature/zf3-compatibility into 413e02f on zf-fr:master.

@weierophinney
Copy link
Collaborator Author

and we have green! Pinging @bakura10 !

"zendframework/zend-view": "^2.8.1",
"zendframework/zend-serializer": "^2.8",
"zendframework/zend-log": "^2.9.1",
"zendframework/zend-i18n": "^2.7.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some cleanup should be done here. I remember that I've added all of this crap in the past due to all those dependencies being needed, but they are actually not used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on this now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I was able to remove all but the QA tools and the module manager dep from the dev rules.

@bakura10
Copy link
Member

Looks good :p. @danizord just in case !

),
);
],
];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still some array() in this file

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably because they're in comments. I can fix if you'd like.

On Aug 16, 2016 5:01 PM, "Daniel Gimenes" notifications@github.com wrote:

In config/zfr_cors.global.php.dist
#38 (comment):

@@ -43,5 +43,5 @@ return array(
* the proper response header.
*/
// 'allowed_credentials' => false,

  • ),
    -);
  • ],
    +];

There are still some array() in this file


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/zf-fr/zfr-cors/pull/38/files/a9bbdf9c81cfe3c14553208bb60d4558683168fa#r75029118,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABlVypBqBBLqregns4F7bGyomA2gXeDks5qgjMtgaJpZM4JloTx
.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please, then I'll be able to merge!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@danizord
Copy link

Looks good for me 👍

- Removed arguments that were unused
  - ensures the signature will work with either zend-servicemanager version
- More defensive about configuration
  - check that the service exists before pulling it
  - check that the key exists before pulling it
- Removed all dependencies that were not required by the package, and verified
  that tests continue to run.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.992% when pulling 0e675a7 on weierophinney:feature/zf3-compatibility into 413e02f on zf-fr:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 97.015% when pulling 166355d on weierophinney:feature/zf3-compatibility into 413e02f on zf-fr:master.

@bakura10
Copy link
Member

Tests not passing :D.

@weierophinney
Copy link
Collaborator Author

@bakura10 Yep - it's because those dependencies, or at least some of them, are needed by v2 components. I'm looking into finding out which.

@bakura10
Copy link
Member

Good luck, I’ve tried in the past, never managed to remove those dependencies ;).

- zend-i18n, zend-log, zend-serializer, and zend-view are needed when testing
  against v2 releases.
@weierophinney
Copy link
Collaborator Author

Got rid of zend-config from there, at least. Rest were needed for testing against v2. I think if you weren't using the module manager in the test bootstrap, we could likely reduce the deps more, but I think that might take some significant re-architecture.

@weierophinney
Copy link
Collaborator Author

Okay, I think I'm done now, @bakura10 !

@bakura10 bakura10 merged commit d6ce360 into zf-fr:master Aug 17, 2016
@weierophinney
Copy link
Collaborator Author

@bakura10 Ping me when the release is tagged, and I'll announce via the zfdevteam and Apigility twitter accounts.

@bakura10
Copy link
Member

Tagged as 1.3.0 :). Thanks!

@weierophinney weierophinney deleted the feature/zf3-compatibility branch August 17, 2016 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants