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

Add machine readable events #12299

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@dawehner
Contributor

dawehner commented Oct 23, 2014

As discussed in [#11878] it would be great to have some simple machine readable way to find events

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #11878
License MIT
@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Oct 23, 2014

Contributor

I'm not really a fan of useless annotations. What about just creating a command that parses all classes that have the "Events" suffix? This would also make it easier to parse 3rd party packages and list optional events if they follow the same system.

Contributor

iltar commented Oct 23, 2014

I'm not really a fan of useless annotations. What about just creating a command that parses all classes that have the "Events" suffix? This would also make it easier to parse 3rd party packages and list optional events if they follow the same system.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 23, 2014

Member

@iltar the idea here is to give a hint to phpdoc generators.

And a command parsing classes ending with Events would have to parse all files in the project to find them, which would be quite inefficient.

Member

stof commented Oct 23, 2014

@iltar the idea here is to give a hint to phpdoc generators.

And a command parsing classes ending with Events would have to parse all files in the project to find them, which would be quite inefficient.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Oct 23, 2014

Contributor

@stof Having 3rd party bundles adapt to this standard will probably even take longer, where as it might be a lot more common for people to name their their classes *Events. I know I have done that and I think doctrine has this as well. Making 3rd party bundles adapt to a documentation/annotation standard is a lot harder and more work than scanning for an already existing pattern.

Annotations also have the side effect of being used by parsers and IDEs. With a common name like Category, who knows what breaks. Who knows what custom Annotation readers will break on this? Side effects will most likely be limited, but in my opinion, a (slow but cachable) file scanner should be a better solution and show more possible events.

I think the goal of this is to not only document Symfony events, because they are already documented, but also to gather all events and mark which are being listened to. On systems that have access to "find", this can even be an extremely fast one liner in bash.

Contributor

iltar commented Oct 23, 2014

@stof Having 3rd party bundles adapt to this standard will probably even take longer, where as it might be a lot more common for people to name their their classes *Events. I know I have done that and I think doctrine has this as well. Making 3rd party bundles adapt to a documentation/annotation standard is a lot harder and more work than scanning for an already existing pattern.

Annotations also have the side effect of being used by parsers and IDEs. With a common name like Category, who knows what breaks. Who knows what custom Annotation readers will break on this? Side effects will most likely be limited, but in my opinion, a (slow but cachable) file scanner should be a better solution and show more possible events.

I think the goal of this is to not only document Symfony events, because they are already documented, but also to gather all events and mark which are being listened to. On systems that have access to "find", this can even be an extremely fast one liner in bash.

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Oct 23, 2014

Contributor

@iltar
I think having 3rd party bundles adapting to that standard is kind of a weak point, given that there is currently not a really standard anyway.

The one big reason why having some annotation (you could also say that its just a ordinary phpdoc) is that doc tools out there already understand this. Parsing by classname in each file is way more specific compared to every common features, for example listing documentation by group.

Contributor

dawehner commented Oct 23, 2014

@iltar
I think having 3rd party bundles adapting to that standard is kind of a weak point, given that there is currently not a really standard anyway.

The one big reason why having some annotation (you could also say that its just a ordinary phpdoc) is that doc tools out there already understand this. Parsing by classname in each file is way more specific compared to every common features, for example listing documentation by group.

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Nov 9, 2014

Contributor

Can someone explain me what bits here are wrong? There is a contribution header up there, changing documentation should also not let travis fail completly! Any idea?

Contributor

dawehner commented Nov 9, 2014

Can someone explain me what bits here are wrong? There is a contribution header up there, changing documentation should also not let travis fail completly! Any idea?

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Nov 9, 2014

Member

@dawehner fabpot is only run when a new commit is pushed. It doesn't detect message changes in the PR description. I assume you didn't have the table when creating this PR?
Also, travis is not failing because of this.
Besides that,

Member

wouterj commented Nov 9, 2014

@dawehner fabpot is only run when a new commit is pushed. It doesn't detect message changes in the PR description. I assume you didn't have the table when creating this PR?
Also, travis is not failing because of this.
Besides that,

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Nov 9, 2014

Contributor

Okay ... rebased so updated.

@wouterj Did you maybe forgot to write down your full sentence.

Contributor

dawehner commented Nov 9, 2014

Okay ... rebased so updated.

@wouterj Did you maybe forgot to write down your full sentence.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Nov 10, 2014

Contributor

@dawehner the thing is, I really don't see why you would want to list all events known from only symfony. They are already documented on the website.. What I _do_find interesting, is being able to list all events available from all my vendors.

Contributor

iltar commented Nov 10, 2014

@dawehner the thing is, I really don't see why you would want to list all events known from only symfony. They are already documented on the website.. What I _do_find interesting, is being able to list all events available from all my vendors.

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Nov 10, 2014

Contributor

@iltar
Indeed this is the point, thank you. With that standard Drupal can also use the standard and mark
the events, same as other bundles out there. Its an opt IN thing

Contributor

dawehner commented Nov 10, 2014

@iltar
Indeed this is the point, thank you. With that standard Drupal can also use the standard and mark
the events, same as other bundles out there. Its an opt IN thing

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Nov 10, 2014

Contributor

@dawehner That's why I think an @category won't work. It's a convention symfony might follow, but it takes a long time for everyone to adapt to this, if they even bother.

Contributor

iltar commented Nov 10, 2014

@dawehner That's why I think an @category won't work. It's a convention symfony might follow, but it takes a long time for everyone to adapt to this, if they even bother.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Nov 10, 2014

Member

Looks like something that does not hurt us, so I'm 👍

Member

fabpot commented Nov 10, 2014

Looks like something that does not hurt us, so I'm 👍

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Nov 12, 2014

Member

@category has a very specific use for phpdocs, so I'm wondering if it would be better to choose something else. What do you think @mvriel?

Member

fabpot commented Nov 12, 2014

@category has a very specific use for phpdocs, so I'm wondering if it would be better to choose something else. What do you think @mvriel?

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Nov 12, 2014

Contributor

I would love to be able to use @group but I haven't found any information about that specific tag on phpdoc. Drupal uses it pretty mmuch everywhere already

Contributor

dawehner commented Nov 12, 2014

I would love to be able to use @group but I haven't found any information about that specific tag on phpdoc. Drupal uses it pretty mmuch everywhere already

@mvriel

This comment has been minimized.

Show comment
Hide comment
@mvriel

mvriel Nov 13, 2014

@fabpot I also think another tag name might be more well suited. With @category people are able to mention a top level grouping above @Package.

Even though it is under discussion for PSR-5, this tag name might still cause some confusion for people who have used docblocks for a long time

mvriel commented Nov 13, 2014

@fabpot I also think another tag name might be more well suited. With @category people are able to mention a top level grouping above @Package.

Even though it is under discussion for PSR-5, this tag name might still cause some confusion for people who have used docblocks for a long time

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Nov 13, 2014

Member

So, what about a @event tag instead? Like Doctrine has an @annotation one.

Member

fabpot commented Nov 13, 2014

So, what about a @event tag instead? Like Doctrine has an @annotation one.

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Nov 14, 2014

Contributor

I would be totally fine with that as well

Contributor

dawehner commented Nov 14, 2014

I would be totally fine with that as well

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Nov 16, 2014

Member

@dawehner Can you make the change? I think we need to use @Event to be consistent with the Doctrine @Annotation.

Member

fabpot commented Nov 16, 2014

@dawehner Can you make the change? I think we need to use @Event to be consistent with the Doctrine @Annotation.

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Nov 16, 2014

Contributor

Sure, just was slacking around

Contributor

dawehner commented Nov 16, 2014

Sure, just was slacking around

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Nov 16, 2014

Member

Thank you @dawehner.

Member

fabpot commented Nov 16, 2014

Thank you @dawehner.

fabpot added a commit that referenced this pull request Nov 16, 2014

minor #12299 Add machine readable events (dawehner)
This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #12299).

Discussion
----------

Add machine readable events

As discussed in [#11878] it would be great to have some simple machine readable way to find events

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #11878
| License       | MIT

Commits
-------

ace9a22 Add machine readable events

@fabpot fabpot closed this Nov 16, 2014

@Haehnchen Haehnchen referenced this pull request Apr 29, 2015

Closed

[Event] @Event annotation completion #493

1 of 1 task complete

@Koc Koc referenced this pull request Sep 4, 2016

Merged

Add annotations #623

fabpot added a commit that referenced this pull request Oct 4, 2016

bug #20156 Fix event annotation for arguments resolving event (Koc)
This PR was submitted for the master branch but it was merged into the 3.1 branch instead (closes #20156).

Discussion
----------

Fix event annotation for arguments resolving event

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12299
| License       | MIT
| Doc PR        | -

Commits
-------

384d0ee Fix event annotation for arguments resolving event
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment