Active
Project:
Coding Standards
Component:
Documentation
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
27 Nov 2010 at 23:27 UTC
Updated:
11 Feb 2025 at 10:11 UTC
Jump to comment: Most recent, Most recent file
Instead of
/**
* Implements hook_help().
*/
we could have
/**
* @implements hook_help()
*/
@implements is actually a standard Doxygen command: http://www.stack.nl/~dimitri/doxygen/commands.html#cmdimplements
api.drupal.org could then list all the implementations of each hook.
| Comment | File | Size | Author |
|---|---|---|---|
| #49 | 983268-49_doxygen_hook_implements.patch | 270.78 KB | wojtha |
| #13 | 983268-13_doxygen_hook_implements_alternative.patch | 269.93 KB | wojtha |
| #9 | 983268-8_doxygen_hook_implements.patch | 258.19 KB | wojtha |
| #9 | 983268-8_doxygen_hook_implements_stats.txt | 9.86 KB | wojtha |
| #3 | 983268-3_doxygen_hook_implements.patch | 259.38 KB | wojtha |
Comments
Comment #1
Josh The Geek commented+1 This would be really cool.
Comment #2
wojtha commentedI got the same idea just couple minutes ago, but I didn't found this issue so I created my own issue: #1150050: Add reference to hook to all core hook implementations
Comment #3
wojtha commentedComment #4
tstoecklerThe functions should still have a one-line summary, I think. So @implements should be added to the functions, not replace the function summaries. In the future we could try to make the function summaries tell something about what they actually do, instead of just which hook they implement. For some hooks this might not make a lot of sense (ie it's rather obvious what "Implements hook_entity_info()" means), but for others this would be really useful (for example "Implements hook_cron()" tells you absolutely nothing about what the function actually does).
Comment #5
jbrown commentedThanks!
There needs to consensus and the coding standards would need to be updated first.
Also, there shouldn't be a full stop at the end of the @implements line.
Comment #6
catchIt's a shame about the function summaries, often hook implementations do several different things, or as jbrown points out don't really need describing. No function summary is probably worse than one that duplicates @implements though, so agree with leaving them in for now. Overall this is great though, would make api.drupal.org than bit more useful.
Comment #7
catchArghh pressed submit too quickly.
When we have variable hook names, do we want to reference both hooks?
i.e. hook_form_FORM_ID_alter() and hook_form_comment_form_alter()?
First thought I'd say yes we do, unless there's an issue with presenting two @implements for some reason.
Comment #8
jbrown commentedNo, the hook that is being implemented is hook_form_FORM_ID_alter() so there should only be a @implements for that.
"hook" gets replaced with the implementer. "FORM_ID" gets replaced with the form_id. These are both parameters of the implementation.
Technically the hook should be called IMPLEMENTER_form_FORM_ID_alter().
Comment #9
wojtha commented@tstoeckler I basically agree but is different issue IMO. This issue aims only to hook referencing using doxygen to provide better API pages for hooks.
@jbrown removed full stops
Added also some statistics, generated using git diff --stat. There are 1003 hook implementations in D8. And I guess it will me more in D7 since many hook_update_N functions were wiped out.
Comment #10
tstoeckler(catch in #6)
I agree totally.
I don't think linking multiple @implements should be a problem per sé, but for instance hook_form_comment_form_alter() won't be linked on api.drupal.org, so I don't know if that's a good idea. Unless we find some way to automatically link to the form builder function but that's probably (while technically possible) very complex. Maybe needs a bit more thought.
I think a patch that simply adds a "@implements foo()" to every "Implements foo()." is a good first step.
Comment #11
sunReplacing Implements with @implements is technically correct. Duplicating makes no sense. Truth is, all of these functions do not have a phpDoc summary. A standardized sentence in the form of a homegrown pattern is not a summary. Truth is, there is no summary. Apps like Druplicon in IRC should rightfully complain about a missing phpDoc summary.
Therefore, I support this patch as is.
Comment #12
tstoecklerI very much like our standard of having a one-line summary with each function. I also think in the long run this makes sense for hook invocations. For instance:
For me the duplication
is only an intermediary step.
Comment #13
wojtha commentedSince I have prepared regex routines in my IDE to provide The Great @implements Migration I created also the alternative patch where @includes are addons, not replacements of the original "Implements..." summaries.
I agree with catch and tstoeckler that there should be always some one-line summary, but I also agree with sun that we should avoid duplication if possible. I think there is a way: What about repeating the summary if the function don't do nothing special?
The patch currently includes some chunks which are not nice, like these:
Example 1
Based on the discussion above in this issue I think it should be
Example 2
"Duplicated" default comment looks really weird, possible solutions: A) removing the standardized sentence, like in the original patch
or B) if the hook doesn't something special we can just simply repeat one-line summary from the original hook, so the summary will saying something meanigfull.
If we go this way, the next step should be fixing the one line summaries and placing the @implements in the right order in the comment block since some hook implementations already have them (see example 1) and if the hook doesn't do something special I will simply add the hook's one-line summary it makes more sense for me than repeating "implements" twice (see example 2). The only down side of this approach which I can see is that if the original hook's summary will change we may need to change also the summary in some of its implementations, but with @implements we will have better idea where we need to provide the change and also we know search and replace magic. :-)
There are 1003 hook implementations in Drupal core which is a lot, but I think it is dooable :-) and worth to do it, since Drupal API is the basement.
Comment #14
jhodgdonPlease let's not do this. We already have an issue in the API module to fix the perceived problem, which is that you can't automatically make a list of functions that implement a hook:
#987450: On a hook page, list functions that implement this hook
Adding a new @tag is probably not the right solution.
Comment #15
tstoecklerI think it is very valuable to have proper function summaries for hook implementations depicting what the functions actually do.
Let's here some more opinions.
Comment #16
jhodgdonWe already have standards in place that say that for hook implementations, we should reference the hook by saying "Implements hook_abc()" in the first line, which accomplishes a link to the hook doc. We don't want to duplicate the hook doc all over the place.
We do want documentation on the specifics of the hook, and that is already in the standards, which say that if there is something specific that needs to be noted, to put it in the description.
If you want to discuss ways to generate lists of functions that implement a particular hook, please do it on the other issue.
I don't see anything else of note here in this issue, so I'd like to leave this closed as a duplicate of the other issue. Or did I miss something? If so, please change the title and add a summary.... I didn't read through all the comments or the entire patch, but the @implements is (in my opinion) not a plus.
Comment #17
tstoecklerOpening this one last time. I still think this could use some more eyes.
The short version is this:
We currently do:
Proposed is the following:
For me personally there are a ton of things which make the latter better, but above there were also people saying the opposite. If there is consensus that what we currently have is better than the proposal, then I'll happily shut up, I just don't see that being the case (yet).
Comment #18
sun+1 for @tstoeckler. Let's give this discussion some more room.
#987450: On a hook page, list functions that implement this hook is solely about the technical API module aspect of linking between related functions. In contrast, the fruitful discussion in this issue is rather about readability and meaning of code, and lastly Drupal coding standards. The implications on API module and api.d.o are not and should not be the dominant factor.
Comment #19
wojtha commented+1 for @tstoeckler (#12, #17) from me too. Changing title to better reflect the current topic of this thread.
I also added some summary what we are actually discussing here to the "parallel" #987450-8: On a hook page, list functions that implement this hook issue.
Comment #20
jhodgdonI am sorry! Without the summary of what you're trying to do in #17, just looking at the patch, I didn't understand the merits of what you were proposing. However, one note: to be in line with the rest of our function standards, the proposal in #17 needs to be:
Note the S at the end of Declares.
Now, in regards to the patch in #13. The patch is full of stuff like this:
The first line here is exactly the same information as the second line. I'm not in favor of this being allowed or encouraged -- it's totally redundant, and I don't see any merit at all in adding the second line, so I wouldn't want to accept a patch that did this. I also do know that it's unlikely someone is going to want to go out and write good one-line summaries for every hook implementation in core. So I would advise against adopting this new standard, for practical reasons.
I also don't really think there is a technical reason we need an @implements tag... is there? What exactly is it buying us?
Comment #21
jhodgdonI would also like to note that the @implements Doxygen tag is semantically supposed to indicate inheritance (in the sense of classes and interfaces). http://www.stack.nl/~dimitri/doxygen/commands.html#cmdimplements
What we are using the word "implements" for in Drupal hook terminology is really not the same thing at all. The individual hook is more like a method override than an interface implementation (in that analogy, the whole module would be "class" that is the "implementation" of the Drupal hook "interface").
Comment #22
wojtha commented@jhodgon yes, it is full of redundant stuff, this one wasn't meant as a final but only as an intermediate step as a reaction to the @tstoecler suggestions. My original patch changed only "Implements" to @implements which will enable native doxygen support for hook implementations (this is what we "are buying"). And as jbrown wrote in the description:
So initially I just replaced the only "Implements" with the "@implements", but catch has interesting suggestion in #6 that
So I created alternative patch which basically duplicates the "Implements" line, but this one was meant as a patch for further discussion (I suggested some possibilites how to remove redundant comments in #13)
Comment #23
jhodgdonBut I don't think we actually want the "native doxygen support for @implements". Take a look at it on the Doxygen site (I put the link in above). It makes a class inheritance diagram, which is not semantically what we really want here.
Comment #24
wojtha commentedYou are right that @implements is meant for classes and interfaces for the first place. There is example what Doxygen provide as an example for that keyword: http://www.stack.nl/~dimitri/doxygen/examples/manual/html/index.html
But I think you are wrong about the thing that hook implementation is override. It isn't. In Drupal, we are overriding only theme functions and templates (in the structural programming code). But hook implementations is equal to "observers" since hook system is based on the "passive observer pattern". Hook could be considered as abstract class or interface with given (and required) input and output.
If we implement the hook in the module, we are basically (passively) registering our "observer" function to the system, we are not overriding some default function.
So I think I'm semantically right :-)
Comment #25
wojtha commented@jhodgdon but we definitely should try and explore if we could use this keyword for the functions ... but I bet it will not be a problem since Dogyxen is very flexible and can be used for many different languages - like e.g. javascript where function is sometime function, sometimes object constructor...
Comment #26
jhodgdonI guess my point is that I'm not convinced that using the @implements keyword buys us any Doxygen functionality that we want. As far as I can tell, it's used in Doxygen to generate class inheritance diagrams for languages without class inheritance. That is not what the proposal here wants to use it for.
And if the whole argument for adopting @implements in the hook doc is so that we could leverage what Doxygen does for @implements, I don't think it makes sense.
Comment #27
jbrown commentedCurrently the documentation for a hook implementation is machine readable in all but name. It doesn't make sense to have such a standardized sentence - it should be a Doxygen command.
"Implements hook_blah()." is not a description of what the function does. Hook implementations should have proper descriptions of what they do like all other functions. It's quite shocking that none of the hook implementations in core actually describe what they do.
It would definitely be stupid to have both "Implements" and "@implements".
@implements should go at the very bottom of the Doxygen block.
@jhodgdon your description in #21 of what a hook isn't (interface implementation) is actually a very accurate description of what it is.
When a module implements a hook or a theme override it _is_ inheritance. A better term than hook is "module method". Hooks are a subset of module methods. Hooks such as hook_install are not actually hooks because they are only called for a specific module at a time.
I support the approach taken in the patch in #9.
Comment #28
jbrown commentedThe documentation for our use of Doxygen for functions already states that
This issue is just about @implements
Comment #29
jhodgdonWhatever. I still don't think we want to generate a "class inheritance" diagram for hook implementations, and I still don't think they are the same thing, but I see your analogy.
So, the patch in #9 just replaces the standard line
Implements hook_whatever().
with the standard line
@implements hook_whatever()
I don't see that as an improvement in readability. If drumm chimes in and says it will make resolving that API issue easier, then I would consider it, but I don't see why we would want to do it just for fun.
Comment #30
jhodgdoncross-post, changing title back
Comment #31
jbrown commentedThere aren't any language constructs in PHP that do what we need for modules. Currently we use magic functions because it is the best compromise.
Any module can 'invent' a method that can be 'implemented' by any other module. Implementations of certain methods are observers (hooks).
At a higher level, certain methods can override others, e.g. a theme can override a default theme function in a module.
'Implements' is definitely the right word and it should be machine-readable.
We have our own implementation of Doxygen in the API module. It doesn't have to be identical to the standard (we are doing weird Drupal things that the standard doesn't cover), but it makes sense to use the standard as much as possible.
If Drupal source code were to be run through a standard Doxygen tool the output may not make perfect sense, but that it okay.
I think what this boils down to is that our documentation should be as semantic as possible. If a function is implementing a method / hook, this is semantic information and it should be published semantically. Hypothetically this can be used by API, Coder, editors, code visualisation software, or something yet unimagined.
I support @sun's comments in #11.
Comment #32
wojtha commentedYes, jbrown, thats exactly what was in my mind when I was writing:
in comment #8 of #987450-8: On a hook page, list functions that implement this hook.
But the truth is that the "Implements" standard has been defined for years and guys in #987450: On a hook page, list functions that implement this hook are now trying to use this "/* Implements hook_xy */" as a semantic information, they want to parse it and provide the list of hooks on the api page as we want too. And I'm just asking why when Doxygen has semantic attribute for that. And that attribute can be used easily by other technologies or source code reflectors and not exclusively by our API module.
So the goal is nearly same, but our path is different and needs change of the Coding Standards but I believe that our approach is more commonly standardized and open to use with other tools like IDE, code reflection etc.
Comment #33
jbrown commented@wojtha could you post the code you used to generate the patch in #9?
Comment #34
wojtha commented@jbrown I lost it but it was something like:
search: "^ \* Implements (hook_[a-zA-Z0-9_]+)\(\)\.$"replace: " * @implements $1"I ran it using Zend Studio/Eclipse.
Comment #35
jhodgdonOK. So we want to use @implements. I'm convinced, and it appears everyone else is too.
So of the two proposed standards, I'm in favor of a standard that puts a descriptive first line, and @implements at the end of the docblock, as in #20. The question would be how to make that happen. The patch in #13 leaves the first lines as Implements xyz() and adds @implements later. That is not good, and the problem with this standard is that we immediately would have a ton of hook implementations that would be out of compliance with yet another doc standard, and therefore a lot of developers would use these as examples instead of reading the doc standards page, and we'd end up with more doc that's out of compliance with the standards.
I'm not in favor of the other proposed standard, which puts @implements as the first line -- it's very odd looking to me as a human reader of .php files. So I don't like the patch in #9 much, which follows that standard.
Thoughts?
Comment #36
wojtha commented@jhodgon yes! :-)
Based on my comment #13, my suggestion is A) place one-line summary of what the hook implementation really does:
or B) if the hook doesn't something special we can just simply repeat one-line summary from the hook documentation, so the summary will be saying something meanigfull.
Comment #37
wojtha commentedMy patch in #13 is the first step toward this approach...
So if you see something like this:
After the "second step", it should look like:
But its a lot of work, there are more than 1000 hook implementions in D7/D8 which needs to be edited, so I wanted to discuss it first before I start working on it.
Comment #38
wojtha commentedSlightly related: #587124: List hooks that a function invokes
Comment #39
jbrown commentedBefore we discuss how to go about patching core, we need to update the documentation, example:
In addition to the standard brief description and optional long description, the docblock of a hook implementation should contain an @implements directive at the bottom:
This generates a link to the hook definition, reminds the developer that this is a hook implementation, and avoids having to document parameters and return values that are the same for every implementation of the hook.
Comment #40
jhodgdonRE #39 - Actually, I don't want to update the documentation section on node/1354 OR adopt this as a standard, until we have a plan to patch core that is viable, because in the past, we've updated standards and then had (a) 99% of the doc out of compliance with the standards and (b) people still writing doc that is out of compliance with the standards, because 99% of the examples in core were out of compliance.
RE #37 - Making the "first step" be to make the documentation read like the patch in #13 -- Nope. If we accept the patch in #13, we will have a lot of bad documentation in core, and people will start copying its example. We need a patch that has good documentation in it. Someone needs to adopt this project and make a patch. When we have a good patch, I'll update the node/1354 standards and we'll adopt the standard.
RE #36/#37 - ALL function doc needs to start with a verb in 3rd person, i.e., ending in an S. Several of the examples you have in there don't. Just pointing this out...
And by the way, this 3rd person verb thing is another standard that we adopted (after much discussion) without a patch. As you can see if you browse the function listings for Drupal 7/8 on api.drupal.org, it has NOT been widely adopted, and there are still many functions that are out of compliance with this standard.
So, just to be clear:
I'm fine with making #39 the standard for hook implementation documentation, and it looks to me as though we have reached consensus on it. So, if someone wants to work on a patch that gets at least most of the core doc into compliance with that standard, I stand ready to review that patch, and update the standard.
Until that patch exists, I think this standard is "a good idea but not adoptable for practical reasons". I'm also not OK with having the interim patch in #13 go into core, because in my opinion, #13 makes the state of core documentation worse than it is currently, and I really don't want to put in an "interim" patch that makes core worse.
[rant coming]
But... really, I think there are many other core doc issues that you all could be spending our time fixing rather than putting time in on changing this standard. I still don't think that updating this standard or working on this patch really buys us very much, in terms of the clarity and correctness of the Drupal core documentation. If all of the people who have commented on this issue would spend some time patching the other existing issues we have in the documentation queue, which deal with documentation that is actually incorrect and really needs a fix, the community would really benefit from having better documentation. We have several pages of documentation issues for Drupal 7/8, not to mention quite a few lingering Drupal 6 issues, and for the last several years, I've been nearly the only person patching them. I could really use some steady help! Just saying...
[end of rant]
Comment #41
wojtha commentedI din't mean it as the patch which should be commited directly, it was something like proof of concept and base for working on it further. I just wanted to share intermediate results and discuss further directions.
I disagree with that ... for me 1) having listed hook implementations on the API pages is a great for having overview where are these hooks used. So it is useful for core development - what will be affected when I modify the hook? or learning how to use the hook from examples. And 2) meaningful one line summary at hook implementation will porvide better insight for developers, especially when they will be directly view and edit the source code, especially for newbies whose don't exactly know what each hook does "Implements hook_xy" is meaningless, its just reference to API. I think for the devs is that issue pretty useful.
Thats your side of view and I understand that, I'm now working for three months on several Openid module patches for D6-D8. Some of them were identified and reported 2 years ago, some of them reports functionality regressions or cut out of some Openid providers, some of them are on the edge of the classification as a security issue ... as a result Openid module is buggy in D6 and pretty much broken in D7+ and nobody cares... Contributing to Drupal core is about scratching own itch or doing for fun. For me the fixing of the openid module is the scratching own itch and this issue is mixture of both: 50:50 scratching own itch and doing something for fun ... As English isn't my native language, I rather spent my spare time fixing PHP bugs... But after we will close this issue (I hope so) I can't rule out that I will be interested more in the documentation issue queue ;-)
Comment #42
casey commented+1 from me.
As I see it you should look at the hook_{foo} functions defined in .api.php files as being interfaces; As PHP does not support the concept of function interfaces we, according to Doxygen's manual, may use @implements.
Like said before hook implementations finally can have a descriptive intro line.
I even think that our "magic callback" functions could use an @implements directive. Instead of defining them in the .api.php files as hook_{foo} we could use another prefix like "callback_" or "routine_". A developer then can easily find out what kind of function he/she is dealing with.
Comment #43
psynaptic commentedIs there any chance this could be done in 7.x too? It's not really an API change, is it? I'm willing to work on this patch if it could be applied to both 8.x and 7.x. We could really use this on Examiner.
This patch should add the description too. I don't really like what #13 is doing. Might as well do it properly, no matter how long it takes.
Comment #44
jhodgdonIf someone makes a good patch for the doc, making it all comply with the proposed standard in #39, we can definitely apply it to both 7.x and 8.x. But someone also would need to make a patch to the API module (and get it deployed on api.drupal.org) so that it does something intelligent with @implements. So this is far from being accepted at this point.
Comment #45
wojtha commented#42 @casey
Do you mean menu loaders or something like that? Or could you give some examples?
#43 @psynaptic
Cool! Glad to hear, as I mentioned several times, the patch in #13 is only intermediate patch, like the basement to build the wall on and also for the purpose like review it and discuss what way to go further... Unfortunately, I'm now pretty busy with the client work and with the OpenID fixes for 7.x which have priority for me, so I had to postpone work on this. Help on this patch will be appreciated, since there are more than 1000 hook implementations in the core 8-)
The patch in #13 is just the first baby step, the second step and the idealistic result is described in #37 and #39.
Comment #46
tstoecklerPlease let's not mix callbacks in here. Not that it's necessarily a bad idea to go a similar direction, but per @jhodgdon's #40 (no offense, just saying :) ) this is already a monster patch as it is now. And we have a clear direction for hooks now, while callbacks would still need to be discussed, because they are a bit different. Let's leave that for #1114032: Call callbacks callbacks, and let hooks all be true hooks, and others.
Comment #47
wojtha commented@tstoeckler +1 I agree, this issue is focused to hooks only.
When I'll have some spare time I will try to make some progress on this. Since this will be real monster patch, thousands lines, If anyone will make some progress, please share you intermediate results.
Comment #48
psynaptic commentedI'm going to continue this using the patch in #13 as a starting point.
Patch in #13 doesn't apply. It asks me for the files to patch, even though I'm using patch -p0 < .
Comment #49
wojtha commented@psynaptic yeah, since we are patching the whole core the patch will faill to apply after each bigger change in the core.
I made 3-way merge against current 8.x branch.
Comment #50
Crell commentedI am extremely -1 here. Are we using Doxygen or not? If so, then using @implements for something other than a class/interface relationship is *wrong*. As in, you're misusing the tools in ways that will only serve to further confuse people not already part of the Drupal cult. As in, you're holding the hammer on the wrong end.
We bastardize our tools in proprietary was enough as is. We should NOT be adding another one, for the love of god...
If we absolutely must make our docblocks bigger for some reason, there is support for defining new @ tags in both Doxygen and PHPDoc, and of course we have our own proprietary parser anyway. Make one called @hook that is specifically for "this implements hook X" and call it a day. Don't abuse the tools. Abuse is not nice and is a felony in most states.
(Besides, we've been moving further toward PHPDoc anyway, which is a move we should continue.)
Comment #51
psynaptic commentedWe definitely need to add proper descriptions to all of these functions. We also need to ensure API and bot module can return the human-readable description, rather than the rather useless "Implements hook_foo()."
Comment #52
aspilicious commented-1 from me @implements totally confused me. See #50.
Comment #53
tstoecklerIf the problem is @implements, what about @hook? E.g.:
Whether we then write "Implements hook_entity_info()" on api.drupal.org or not is then a secondary matter.
Comment #54
Crell commentedWe should check to make sure @hook has no existing standing meaning in either PHPDoc or Doxygen. Assuming it doesn't, I'm fine with #53.
Comment #55
aspilicious commentedDoes this mean we have to rewrite *all* of the *Implements hook_x* for D8?
Will this not confuse people working on D7 stuff? This will lead to mass confusion and broken tutorials, books and stuff. Is it not better to provide a drupal 8 coding standard page. And link to that page in the current coding standards ==> if you're working on D8 stuff please look at this page.
Comment #56
wojtha commented#55
Yes, it means to rewrite all the *Implements hook_x* for D8. 2nd goal of this issue is also to replace "meaningless" *Implements hook_x* comments with one-line summaries what the hook implementations really do.
I don't think it is confusing. I found more confusing that above most of the hook implementations the one-line summaries or description are missing and the thing that hook implementations aren't listed below the hooks in the api.d.o. The coding standards should be updated once we got this to RTBC state or at least we agree on some keyword which will be ok for the majority of developers and has no conflicts in the PHPDoc and Doxygen. Thanks for noting it.
Comment #57
aspilicious commentedBut all books and tutorials will have standards that are not up to date... I don't think we should force this standard into D7 or D7 contrib.
Comment #58
wojtha commented#53, #54 I searched through Doxygen Reference Manual v 1.7.4 and PHPDocumentator Manual v 1.4.3 and found no "@hook" inside. This keyword is safe.
But I really liked how was the
@implements hook_XYunderstandable for "humans" (unfortunately not for the phpdoc or doxygen parser) and how it losslessly replaced the current standard "Implements hook_XY" line .@hook hook_XYlooks bit weird, but if we'll not found anything better I'm ok with this one too (together with proper the proper one-line summaries).@uses hook_XYis reserved in PHPDoc,@implementshook hook_XYor@implementationof hook_XYis too long, but what about@implementation hook_XYor@apply hook_XYor@utilize hook_XYor@remarks hook_XY. I want to make sure there is really no better keyword than @hook.I took http://www.synonyms.net/synonym/implement as an inspiration source...
Comment #59
Josh The Geek commented#1159356-9: Better error handling: This is a comment by sun on grn about coding standards that would be useful to know here:
Comment #60
tstoecklerNote that this issue is about establishing a standard and enforcing it in Drupal 8 core. While I personally don't share your objections to a backport, that is secondary, at least at this point.
I checked and neither Doxygen nor PHPDoc have @hook. For reference: @implements is only in Doxygen.
@Crell: I just now realized that you proposed @hook in #50. Sorry for making it sound like my idea.
Edit: massive cross-post!
Re #59: The quote from sun speaks directly against #40. So who is right, sun or jhodgdon? :)
Comment #61
wojtha commented#57 I didn't see any problem with the tutorials... most of the old tutorials isn't compatible with the current API. Drupal isn't backward compatible which gives us the power to change the things and make the Drupal better... even the docs. "Implementation line" was changed at least once. In Drupal 6 it was standardized as "Implementation of hook_XY" sice Drupal 7 the standardized format is "Implements hook_XY". So there could be significant change, at least in the next major version.
I think we can enforce it for D8 since it is at least one year ahead. For Drupal 7 we could leave the "Implements hook_XY" as it is and just add the "@hook (or whatever) hook_XY" and one-line summary. In case that this kind of change is allowed in the current stable major version. But e.g. in Examiner they really wish this in D7 since they'll really profit from it. (see #43).
Lets make a balance:
Pro:
- descriptive comments what hook implementations really do - more readable code, no need to look inside the functions or searching for the hook descriptions
- hook implementations listed in the API docs
Con:
- change of Drupal Coding Standards
- update of tutorials for D7(?) there are no docs/tutorials for D8 I assume
Edit: crosspost, wow, this issue is hot
Comment #62
Josh The Geek commentedSorry, I didn't look that far back. jhodgdon, probably, since she's co-head of docs.
Comment #63
jbrown commentedWe are using the term 'hook' too broadly. hook_watchdog() is a hook because it follows the observer pattern. hook_install is not a hook because it is always invoked for a specific module.
I think a better term is 'module method', of which 'hook' is a subset. For this reason I am -1 on using @hook.
Comment #64
Crell commentedjbrown: #1114032: Call callbacks callbacks, and let hooks all be true hooks
Things that are not "true hooks" should be changed to something else anyway. I'm hoping to replace most of them with plugin classes before D8 ships. :-)
Comment #65
jhodgdonWe are technically not using Doxygen or PHPDoc, but our own API module, which is based on Doxygen with PHPDoc syntax... So we can do whatever we want, more or less.
I still think this entire project is much lower priority than any number of other documentation issues, and I'm sorry so many people are spending so much time on it, when they could be fixing the higher-priority documentation issues, like the fact that there are still about 30 issues tagged "Needs Update Documentation" because the 6/7 module/theme upgrade guides are incomplete. Which is the highest priority doc issue currently, in my opinion, since there are still tons of contrib/custom themes and modules out there that people haven't updated.
As a reminder, this issue is NOT going to be fixed any time soon, anyway. Someone will need to update the API module so it can handle @implements, @hook, or whatever before we can even accept the patch of Drupal Core files for 8.x, and just writing the patch is a huge amount of work.
Comment #66
ro-no-lo commentedtstoeckler pointed out that I douplicated (kind of) this issue here #1692618: Hook PHPDoc Comments with clickable references to the hook itself..
I tried @implements hook_menu(). in my IDE and it does nothing for me. I don't really care about the parseing on api.drupal.org. I just care in my issue about the useage in the IDEs of the users. How can I help them to assist me better. Assumed that the IDEs know how to parse Doxygen and PHPDoc.
If you introduce a new @hook, then "no" IDE will know this and can't (maybe) link to the hook definition. I use my IDE daily and not so much the drupal.org linkage. Anyway please consider my issue in relation to this one to support both ideas of a better hook_foobar(). documentation. #1692618: Hook PHPDoc Comments with clickable references to the hook itself.
Comment #67
jhodgdonI closed that other issue as a duplicate -- we shouldn't discuss solutions to the same underlying problem in two places.
I will just note here that the other issue suggests two solutions:
Ugly:
The first suggestion is not going to work for the API module (or any standard doxygen), since @see lines have to be on their own.
Comment #68
ro-no-lo commentedI think the usecases were different, but anyway. I filed an issue in the PhpStorm Tracker if someone is interested in voting there as well: PhpStorm Issue Tracker
Comment #69
jhodgdonYes, the use cases are different, but this issue is for discussing whether/how to update the standards for documenting hook implementations. Discussing it in two places and coming up with two standards would be rather silly. Perhaps you'd like to add some notes to the issue summary here to make sure IDE concerns are part of the "problem" section?
Comment #70
ro-no-lo commentedI still come back to this issue here and I am thinking that the drupal community is to much focused on api.drupal.org. Drupal which origins from a time where there were no such thing as PHP coding standard introduces more and more non-PHP-standard things on all fronts. This is bad. Examples are the 2 space indentation, .module, .install, .younameit files (So many IDEs do not recognise these files as PHP files and have to be setup for just drupal), TRUE, FALSE, NULL as mandatory uppercase, a freaking complicated directory / files structure, thus a simple autoloader is impossible (therefore we now have files[] arrays in .info files), PhpDoc / Doxygen documentation which is highly customized because of parsing for API.drupal.org. And now there is a disscussion about @hook, so that we can link accross api.drupal.org.
This is so narrow minded that it hurts. First, as I mentioned, IDEs which use the strict Doxygen / phpDoc API / implementation from the original website are pointless, because of the customized Drupal Docs. They therefore cannot correctly assist developers in editing drupal files. Creating my own Doxygen Doc with the software FROM the Doxygen website will fail as well, because I do not (I guess) have access to the customized drupal.org API parser.
If this will continue in D8 nobody outside drupal.org will be able to create API Documentations for his modules and classes, because of newly introduced parameters or keywords only drupal.org understands.
Drupal is like Amercia on this part. The world uses SI units everybody understand except the USA. Bah!!!
Hooks should have a simple:
or
This will be paseble by PhpDoc and Doxygen every modern IDE out there and api.drupal.org should be easily use that for it's advantage as well to link accross API pages. I mean the rules to write hooks are crystalclear. A parser can check for that syntax, if it sees a reference to a function starting with hook_ in the documentation.
Comment #71
sun@ronan.orb: Please leave your unsolicited rants at home. You're not helpful, at all. That kind of attitude is anything but productive. It's a bit shocking that even after 3.5+ years of participating in the community, you haven't learned anything since your first post (your entire post history until today doesn't really look any different). Your rant is inappropriate, partially off-topic and outdated, and full of disrespect of the hard work of others. You may disagree and you may articulate your standpoint with technical reasons. But please refrain from bitching and bashing on others. Thanks.
Comment #72
ro-no-lo commented@sun: I did not bashed someone in particular and I'm not judging someone on it's "entire" post history. I'm just concerned. You on the other hand are "slightly" disrespectful of other decisions like here http://buytaert.net/jennifer-hodgdon . Anyway, I backed up my point with a true list of drupal customisations, which make it indeed harder for the drupal loving php developers to work with drupal, which does not need to be the case. Simple example: why not foo.module.php instead of foo.module. I just want to put some weight on my point that there are like 50000+ developers with different IDEs out there and only one api.drupal.org. My daily work is in my IDE. If my ide helps me to code faster, I might fix faster modules, bugs and contributed modules as well.
As far as I see it the initial post on this wanted to have a page where all implementations of a hook can be displayed. So far so good. Until now there seems to be no conclusion which way to go. To missuse @implements (which as pointed out is exclusive for interfaces) or introdce a new @hook, which no Documenter understands, but drupal.org. I think you may understand my concern about this, because with that in mind also many IDEs will not recorgnise this new @hook and cannot provide assitance to link to the original hook docmentation via hovering mouse. So my intention therefore is - still - to catch two flys at once and collect arguments to stay with the (external) standards instead of introducing new drupal standards.
As pointed out here #1706250: Let's Make Drupal.org Search More and More Useful also me has the strong wish for better search and documentation on drupal.org websites. This post indeed seems to back up your view on my issues, but in the end I think I have no power to change something on *.drupal.org websites. At least nothing at the search. I have no experience with websites of that traffic magnitude. So I just *wish* for certain thing - for a better experience. Just ideas.
Anyway glad that you read through all my posts since the beginning. When you even thing that my reports like this #1672282: Infinite redirect when superadmin and path not found are not helpful at all, then I truely have to rethink my contributions to drupal.
ps: further personal stuff in #drupal.de please. Highlight *sym*.
Comment #73
jhodgdonCoding standards decisions are now supposed to be made by the TWG
Comment #74
tizzo commentedMoving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.
Comment #75
tizzo commentedMoving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.
Comment #76
jhodgdonThis needs a new issue summary, because api.drupal.org already lists the implementations of hooks, so that motivation is not valid any more.
Comment #77
donquixote commentedCurrently hooks, theme functions, form builder functions and maybe others (Drupal 7, primarily) give me a lot of IDE warnings about unused parameters.
For most of Drupal 7 core and contrib this is a lost case, the IDE is complaining all over the place.
But it would be great if people who care could at least make their custom and contrib modules 100% IDE-friendly.
Could we maybe convince the community (phpDocumentor, IDE developers, maybe FIG?) to support an @implements tag?
Besides Drupal hooks, the @implements tag could be used on functions, static methods, closures, or even object methods, to indicate that this function must have the same signature (or weaker) as the function it implements. So the equivalent to "CLASS implements INTERFACE" on procedural or function level. This can still be useful even in a post-procedural era.
This way, the IDE can:
- Complain if the signature does not match.
- Stop complaining about unused parameters.
Within Drupal (7), this would mean:
- Hooks can be documented with "@implements hook_foo()".
- Theme functions can be documented with "@implements theme_HOOK()" or similar, where we would have to provide an example theme function.
- Form builder functions can be documented with "@implements FORM_BUILDER()".
We could even reserve a namespace for these example implementations, to avoid name clashes. So it would be "@implements \Drupal\Api\FORM_BUILDER()". Or it could be static methods, like "@implements \Drupal\Api::FORM_BUILDER()".
The way I imagine it for now, this would all be optional within Drupal, but could be used by contrib authors who want fewer Drupalisms and a happy IDE.
Comment #78
donquixote commentedhttps://github.com/phpDocumentor/phpDocumentor2/issues/1689
Comment #79
Crell commenteddonquixote: I'd suggest bringing that question to FIG as well, as in theory we'er still working on PSR-5, which should succeed the phpDocumentor de facto standard. (Although the Editor is also the lead for phpDocumentor, so I guess now he knows about it. :-) )
Comment #80
donquixote commented@Crell: Good idea :)
https://groups.google.com/forum/?fromgroups=#!topic/php-fig/49ZfuJ1iisg
Comment #81
indrapatil commentedComment #82
indrapatil commentedComment #83
nicxvan commentedI think this will go in the opposite direction:
#3483544: Remove Implements comments once the api can pickup hook implementations from classes.
Comment #84
quietone commented