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

Detect deprecated files #11

Closed
Rarst opened this issue Mar 21, 2013 · 25 comments
Closed

Detect deprecated files #11

Rarst opened this issue Mar 21, 2013 · 25 comments

Comments

@Rarst
Copy link
Contributor

Rarst commented Mar 21, 2013

There is number of deprecated files that seem to only be marked as deprecated by _deprecated_file() function call. Currently this is not reflected in parser output, so for example there is no way to know classes/functions defined in those files are deprecated (since they themselves are not marked as such).

Needs either:

  • manually maintained list of deprecated files to use when processing parsing results
  • keep track of files that call _deprecated_file() on parser level
@JDGrimes
Copy link
Contributor

JDGrimes commented Mar 5, 2014

Maybe the best solution is to add the @deprecated tag to these files? Though that tag is only meant for structural elements, so it normally wouldn't apply to files, I guess.

@GaryJones
Copy link
Member

Are all the structural elements inside "deprecated files" considered individually deprecated?

If so, then the docs for each of those elements should be updated to include @deprecated. Then the parser can just treat them as it would a deprecated element inside a "non-deprecated file".

@JDGrimes
Copy link
Contributor

JDGrimes commented Mar 7, 2014

I've looked at all of the deprecated files, and it appears that any structural elements they hold are properly marked with the @deprecated tag. Third-party elements aren't marked as deprecated, but they shouldn't be, should they?

List of files that are deprecated but aren't marked with the @deprecated tag:

/wp-includes/rss-functions.php
/wp-includes/registration.php
/wp-includes/registration-functions.php
/wp-includes/class-snoopy.php

I think we should open a ticket on track to have these marked with the deprecated tag. The other deprecated files are.

@DrewAPicture
Copy link
Member

@JDGrimes: Just let me know specifically what you need done and I'll move it to the top of the inline docs team's priority list.

@JDGrimes
Copy link
Contributor

JDGrimes commented Mar 7, 2014

@DrewAPicture: Having second thoughts about whether anything needs to be done. I think that all of the structural elements in the deprecated files are marked as such. I think that was @Rarst's main concern here. The @deprecated tag could be added to the four files I listed above so that the parser could easily detect that they are deprecated. But as files are currently saved as a taxonomy, and therefore none of the metadata for them is imported, I don't know that it really makes any difference.

@Rarst
Copy link
Contributor Author

Rarst commented Mar 7, 2014

I am not sure about "individually deprecate" as universal solution here.

See Snoopy for example. It's old and third party dependency, why mess with its inline documentation for the sake of marking it deprecated in WordPress?

@JDGrimes
Copy link
Contributor

JDGrimes commented Mar 7, 2014

If we want to indicate that third-party elements are deprecated in WordPress, then we do need to have the files marked @deprecated. I think that is a better solution than making the parser look for _deprecated_file().

@Rarst
Copy link
Contributor Author

Rarst commented Mar 7, 2014

Why do double effort? _deprecated calls are already in place and perform a function (throwing errors, firing hooks), so they can't be discarded in favor of inline doc. And since they are already in place why not just use that information, instead of duplicating it by adding inline doc to every instance?

@JDGrimes
Copy link
Contributor

JDGrimes commented Mar 7, 2014

@DrewAPicture Does the inline docs team have an opinion on whether files that are deprecated should have the @deprecated tag? As I mentioned above, I'm not sure that it is intended to be used with files, but it is used for most deprecated files in core. So right now the usage is inconsistent.

@Rarst
Copy link
Contributor Author

Rarst commented Mar 7, 2014

PSR-5 is unclear about @deprecated for files, it only mentions applying it to "structural elements", which doesn't include files. So I'd say, if without making any assumptions, files are not supposed to be using @deprecated tag.

@DrewAPicture
Copy link
Member

@JDGrimes If the entire file is deprecated, its file header should hold an @deprecated marking it as such.

@Rarst I'd like it if we could blanket-mark a file deprecated by adding the tag to the file header. Is this something we can support? Also, per your earlier question about external libraries, I was always under the impression that we'd skip parsing those files altogether. If there isn't an "excluded files" list, perhaps there should be.

@JDGrimes
Copy link
Contributor

JDGrimes commented Mar 8, 2014

@JDGrimes If the entire file is deprecated, its file header should hold an @deprecated marking it as such.

OK. Those four files don't.

@Rarst I'd like it if we could blanket-mark a file deprecated by adding the tag to the file header. Is this something we can support?

We already have elements inherit the @package tags from a file, so it wouldn't be hard to have them inherit the @deprecated tag as well. PSR-5 doesn't indicate that it is an inherited tag though. And I think all elements are marked with an @deprecated already, except for the external libraries.

Also, per your earlier question about external libraries, I was always under the impression that we'd skip parsing those files altogether. If there isn't an "excluded files" list, perhaps there should be.

I was thinking the same thing. I don't think that we need to provide documentation for the external libraries that WP uses. On the other hand, we will want their existence to be documented, and if they are deprecated, we'll want that recorded as well. But maybe that shouldn't be part of the parser's job?

@Rarst
Copy link
Contributor Author

Rarst commented Mar 8, 2014

Also, per your earlier question about external libraries, I was always under the impression that we'd skip parsing those files altogether.

No. If it's in core it's in core. There is nothing "external" about the way WP handles libraries, essentially hardcoding them into source. If they are in — they must be in reference.

@GaryJones
Copy link
Member

I've read the comments in this thread several times over trying to decide what I think would be best. For me, at the moment, I'm favouring:

  • searching for existing _deprecated_file() calls as being the canonical definition of whether WP considers the file to be deprecated or not.
    • The call often contains a version number and refers to a replacement file (as opposed to a textual "Use SimplePie instead."), especially when the deprecation is due to a file rename (e.g. rss-functions.php to rss.php).
    • If the file contains nothing else other than an empty line after that _deprecated_file() call, then I see no benefit in including it in the export. An example of this would be registration-functions.php and registration.php.
    • When there's a _deprecated_file() call on an External / non-WordPress @package (such as Magpie RSS), then don't parse anything more of that file, and treat it as empty as above. Something like Snoopy might need file-level documentation updating to add those if necessary.
  • Removing all @deprecated file-level headers tags, as it's duplicating the call, and it doesn't appear to be supported within PSR-5.
    • A quick search suggests that the concept of a "deprecated file" is not one that is popular within the PHP community at large, so I wouldn't expect support for it in documentation standards.

There is nothing "external" about the way WP handles libraries, essentially hardcoding them into source.

Sure, but some are documented as @package External and some aren't. That might be a separate ticket to fix those.

If they are in — they must be in reference.

If everyone thought that about all code, then phpDoc tags like @internal and the de facto @ignore wouldn't exist, along with it's opposite, @api. We're not trying to recreate the source from scratch here, and there's some bits, like empty files or files that have been deprecated for (nearly) 9 major versions that don't need to be cluttering up the documentation reference. Clarity is better than the kitchen sink.

@Rarst
Copy link
Contributor Author

Rarst commented Mar 8, 2014

I stick with my position — everything in is in. The prime major flaw of Codex as reference is and always was poor completeness of information.

As a developer I don't want someone else to decide what I should and should not see, especially in giant code base with long history of inline documentation issues (no offense to recent docs efforts, they are great and welcomed improvement on the situation).

This reference isn't being built at shallow APIs levels, it should be able to serve core developer/contributor as well as novice.

I suggest leaving information out is discussed as separate dedicated issue if there is interest.

@GaryJones
Copy link
Member

The prime major flaw of Codex as reference is and always was poor completeness of information.

Sure, but now we've got something automated, and the docs are far more complete than they've ever been for WP. That gives us the flexibility to sensibly decide what to exclude, rather than playing catch up on what should be included. We're approaching it from a different perspective.

As a developer I don't want someone else to decide what I should and should not see

No, but as someone producing the (tool that creates the) documentation, you should be deciding, and a blanket on "everything" is only helpful to the top of the developer pyramid (and they've got source to look through...).

it should be able to serve core developer/contributor as well as novice.

If a core developer / contributor wanted to see the dirty details, then the source is always going to be better to look through than the documentation scraped from it. As such, I would expect to see DevHub as the place for novice and intermediate developers, who don't favour going through the source, to understand how WordPress works now, not how it used to work.

Does the DevHub project as a whole have a mission statement / aim / goals listed where this is / can be clarified?

@Rarst
Copy link
Contributor Author

Rarst commented Mar 8, 2014

You are derailing discussion of deprecated here, as above - please open separate dedicated issue if you are interested in discussing this in detail.

@GaryJones
Copy link
Member

It's all related - whether everything, including all things marked as deprecated, should be included or not. It was your comment that started the discussion btw; I tacked on a response to it on my post that outlined a suggestion for tackling the deprecated issues.

@Rarst
Copy link
Contributor Author

Rarst commented Mar 8, 2014

Regardless of DevHub content goals my personal goal for Parser is to parse everything. Not necessarily achievable goal in scope needed for DevHub, but still.

We can argue that it's not a priority to be implemented as it's not needed for DevHub, but parsing deprecated, internal, and whatever else things is happening at some point.

So as I see it what-DevHub-displays is entirely separate discussion, likely not even on this issue tracker.

Scope of this ticket:

  • implementing parsing deprecated files
  • pondering if anything needs to be required/recommended to docs team about them

@GaryJones
Copy link
Member

+1 to all of what you just said and point well-made.

My apologies for mixing up the WP-Parser project and the DevHub project.

@JDGrimes
Copy link
Contributor

Note that it would be possible to do this by checking for the use of _deprecated_file() once #79 is complete, assuming that we store uses information for files and not just other functions (I don't know if that has been discussed).

@Rarst
Copy link
Contributor Author

Rarst commented May 23, 2014

Hmm... We currently have file taxonomy for definitions, so we cannot reuse it for calls, taxonomies cannot do two different kinds of relationship.

If we go with post relationships it does make sense to me that files should be CPT with two relationships — what is defined inside of them and what is called inside of them.

@bobbingwide
Copy link

Hi, once again, I'm following this discussion while working on a parallel project, rather than contributing. Having parsed WordPress code with WP-Parser and my own solution I realised I needed to be able to parse the logic within files which wasn't part of a class, method or function. To do this I needed to create a "file" CPT. It's similar to an API ( method or function ) in that it can call functions, invoke hooks and even associate functions to hooks.
The post meta fields that I have for a file are as below.

bw_register_field( "_oik_file_name", "text", "File name" , array( "#length" => 80 ));
bw_register_field( "_oik_api_plugin", "noderef", "Plugin ref", array( "#type" => "oik-plugins" ));
bw_register_field( "_oik_file_passes", "numeric", "Parse count", array( "#theme" => false ));
bw_register_field( "_oik_file_deprecated_cb", "checkbox", "Deprecated?");
bw_register_field( "_oik_api_calls", "noderef", "Uses APIs", array( "#type" => "oik_api", "#multiple" => true, "#optional" => true, '#theme' => false ));
bw_register_field( "_oik_api_hooks", "noderef", "Uses hooks", array( "#type" => "oik_hook", "#multiple" => true, "#optional" => true, '#theme' => false ));

As you can see I have a deprecated checkbox. BUT setting its value is much lower on my priority list than parsing the code to populate the relationships to APIs and hooks.

The next stage will be to handle file relationships - allowing navigation through require(), include() and helper functions that themselves perform file loading.

To achieve this I believe I will have to develop some rule based logic, similar to that in makepot.
These rules could then be extended to fire action hooks for _deprecated related functions.

@Rarst
Copy link
Contributor Author

Rarst commented May 25, 2014

Docs team decided not to use @deprecated for files, so we are definitely back to detecting _deprecated_file() calls.

@Rarst
Copy link
Contributor Author

Rarst commented Oct 2, 2014

Fixed by #133.

@Rarst Rarst closed this as completed Oct 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants