Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

ZF3 #213

Merged
merged 28 commits into from
Jun 27, 2016
Merged

ZF3 #213

merged 28 commits into from
Jun 27, 2016

Conversation

samsonasik
Copy link
Contributor

cherry-pick #210 with incorporated feedbacks.

array($this, 'onCollected'),
Profiler::PRIORITY_TOOLBAR
);
$this->listeners[] = $events->getSharedManager()->attach('profiler', ProfilerEvent::EVENT_COLLECTED, array($this, 'onCollected'), Profiler::PRIORITY_TOOLBAR);
Copy link
Member

Choose a reason for hiding this comment

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

can we use please short array syntax?

@michalbundyra
Copy link
Member

michalbundyra commented Jun 24, 2016

Can we also add to scope of this update for ZF3 following improvements:

  • reduce one lvl directory in src (remove ZendDeveloperTools)
  • use PSR-4 autoloader in composer
  • update phpunit to version 5
  • rename directory with tests (to test) to be consistent with other ZF libraries
  • update .travis.yml (checks for lowest, locked, latest ... )
  • add composer.lock
  • add phpcs checks
  • use short array syntax
  • add trailing comma in multiline arrays
  • add some documentation ?

matrix:
allow_failures:
- php: 7.0
- ./vendor/bin/phpunit
Copy link
Member

Choose a reason for hiding this comment

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

one empty line at the and of the file plz

@samsonasik
Copy link
Contributor Author

@webimpress some feedbacks incorporated. I think phpcs checks and some documentation should be in separate PR :)

}

Copy link
Member

Choose a reason for hiding this comment

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

remove white characters please

@samsonasik
Copy link
Contributor Author

@webimpress updated.

@samsonasik
Copy link
Contributor Author

@webimpress implemented.

@michalbundyra
Copy link
Member

michalbundyra commented Jun 25, 2016

@samsonasik good work! 😄 Thanks!

Can we update please .travis.yml to test library over different version of php and different packages version (lowest, locked, latest), as we have here (for example):
https://github.com/zendframework/zend-view/blob/master/.travis.yml#L25-L69

I'd like to have also trailing commas in arrays (once you changed it to short syntax).

What do you think? BTW. Why you think phpcs checks should be separate PR?

@samsonasik
Copy link
Contributor Author

The original PR is to get it support for zf3.

Original reasons: I have no chance to add phpcs check right now ( need to play with my sons ) :)

Warm regards,

Abdul Malik Ikhsan

Pada 25 Jun 2016, pukul 13.59, webimpress [email protected] menulis:

@samsonasik good work! 😄
Can we update please .travis.yml to test library over different version of php and different packages version (lowest, locked, latest), as we have here (for example):
https://github.com/zendframework/zend-view/blob/master/.travis.yml#L25-L69

I'd like to have also trailing commas in arrays (once you changed it to short syntax).

What do you think? BTW. Why you think phpcs checks should be separate PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@samsonasik
Copy link
Contributor Author

@webimpress my children finally sleeping, I've finally have a chance to add php-cs check and checks for lowest, locked, latest in .travis.yml ;)

@samsonasik
Copy link
Contributor Author

@webimpress travis now green with php-cs check and lowest, locked, and latest check dependencies included \m/

/cc @Ocramius @weierophinney

@michalbundyra
Copy link
Member

@samsonasik very nice 😄 thanks! I think "locked" is not working properly because we don't have composer.lock in the library. Could you add it, please? (Please remove it from .gitignore). Thanks!

@samsonasik
Copy link
Contributor Author

@webimpress done ;)

@samsonasik
Copy link
Contributor Author

@webimpress add composer.lock make travis failure, suggestion ? or could you make a patch to my branch ?

@samsonasik
Copy link
Contributor Author

@webimpress nvm, travis has been fixed now and green ;) 💃 💃 💃

@michalbundyra
Copy link
Member

@samsonasik I'm not sure about this phpcs configuration and used package. Is it PSR-2?
I just had a look on one recently released zfcampus package:
https://github.com/zfcampus/zf-development-mode/blob/master/composer.json#L24
and its configuration:
https://github.com/zfcampus/zf-development-mode/blob/master/phpcs.xml

The same library (squizlabs/php_codesniffer) is also used in some zf libraries, for example:
https://github.com/zendframework/zend-code/blob/master/composer.json#L23
and its configuration:
https://github.com/zendframework/zend-code/blob/master/phpcs.xml.dist

Please notice also nice scripts section in composer.json file:
https://github.com/zfcampus/zf-development-mode/blob/master/composer.json#L38-L48

I've also checked zend-mvc library and there we have fabpot/php-cs-fixer with scripts configuration:
https://github.com/zendframework/zend-mvc/blob/master/composer.json#L54-L64

In some library I saw also friendsofphp/php-cs-fixer... I don't know what we should use. There is completely no consistency between zf libraries. It would be nice to have the same rules, checks and used libraries in all zf modules and the same scripts section in composer.json as well.

BTW. Do we still need PHP 5.5 support in the library? ZF3 is gonna support PHP 5.6 and PHP 7.0.
Can we also add tests on hhvm (with allow failures), as it is in other libraries? I wonder when .travis.yml will be updated to run tests also on PHP 7.1 alpha ;)

@michalbundyra
Copy link
Member

It will be very nice to add this library also to new Zend Skeleton Application (using zend-skeleton-installer). Not sure if it will be possible to auto copy the configuration file?
Of course, this library have to be released first :)

@samsonasik
Copy link
Contributor Author

I think phpcs more enhancement should be separate PR, it's already too far :)

For php 5.5, yes, we need support as we will still uses in zend-mvc 2. This PR used to support both zf-mvc 2 and zf-mvc 3.

Warm regards,

Abdul Malik Ikhsan

Pada 26 Jun 2016, pukul 07.28, webimpress [email protected] menulis:

It will be very nice to add this library also to new Zend Skeleton Application (using zend-skeleton-installer). Not sure if it will be possible to auto copy the configuration file?
Of course, this library have to be released first :)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@@ -22,10 +21,10 @@
* @author Mark Garrett <[email protected]>
* @since 0.0.3
*/
class EventLoggingListenerAggregate implements SharedListenerAggregateInterface
Copy link

Choose a reason for hiding this comment

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

possible BC break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it tested with EventManager v2 and v3

Copy link
Member

Choose a reason for hiding this comment

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

@svycka Essentially, that interface goes away with v3, so to make code forwards compatible, you cannot directly implement the interface. Since this particular class implements a method for attaching listeners to the shared manager, and that method is what is called inside the Module class, it continues to work perfectly.

@weierophinney
Copy link
Member

If I do a new minor or major release of this, we can bump to 5.6 support. 😄

Thanks a TON for the work on this @samsonasik , and for the constant, constructive feedback, @webimpress !

@weierophinney
Copy link
Member

Oh, goodness. Evidently we did a release candidate in October, but never a stable release following.

I'm going to tag that as 1.0.0, and then this will become 1.1.0.

@weierophinney weierophinney added this to the 1.1.0 milestone Jun 27, 2016
@weierophinney weierophinney self-assigned this Jun 27, 2016
weierophinney added a commit to weierophinney/zend-developer-tools that referenced this pull request Jun 27, 2016
@weierophinney weierophinney mentioned this pull request Jun 27, 2016
@weierophinney
Copy link
Member

I just discovered one fairly big issue: zend-version is deprecated, and we are no longer updating it for any but LTS releases. I'll see if I can remove that from the toolbar.

@weierophinney weierophinney merged commit fdaaf23 into zendframework:master Jun 27, 2016
weierophinney added a commit that referenced this pull request Jun 27, 2016
@samsonasik samsonasik deleted the zf3 branch June 27, 2016 21:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants