WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 7 years ago

#14726 closed defect (bug) (wontfix)

PHPDoc @license Tag Review

Reported by: hakre Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0.1
Component: Inline Docs Keywords: has-patch 3.2-early
Focuses: Cc:

Description

Syntax as specified. Helps to normalize as well.

Attachments (5)

14726.patch (5.3 KB) - added by hakre 9 years ago.
14726.2.patch (11.9 KB) - added by hakre 9 years ago.
2nd iteration, tagging untagged files. (fixed encoding)
14726.3.2.patch (10.6 KB) - added by hakre 9 years ago.
14726.3.patch (10.6 KB) - added by hakre 9 years ago.
14726.4.patch (10.1 KB) - added by hakre 9 years ago.

Download all attachments as: .zip

Change History (20)

@hakre
9 years ago

#1 @hakre
9 years ago

Related: #14727

#2 follow-up: @jacobsantos
9 years ago

Second patch looks good except for a few issues:

  • Added subpackage to PemFTP files, and I suppose to fix the Pure class and the Socket class from not showing up in nightlies. The fix for this is simple without adding subpackage and should be fixed in a new ticket.
  • The patch is dirty in some places making changes not related to the @license phpdoc tag. Some examples include the UTF8 translation to ANSI function.
  • Patch doesn't appear to be accepted format for which Trac can read. I'm not sure why this is, but it would be helpful.

#3 in reply to: ↑ 2 ; follow-up: @hakre
9 years ago

Replying to jacobsantos:

Second patch looks good except for a few issues:

  • Added subpackage to PemFTP files, and I suppose to fix the Pure class and the Socket class from not showing up in nightlies. The fix for this is simple without adding subpackage and should be fixed in a new ticket.

I don't understand what that means, but if it's easy to fix, good :)

  • The patch is dirty in some places making changes not related to the @license phpdoc tag. Some examples include the UTF8 translation to ANSI function.

Do you mean character encoding in /wp-includes/class-snoopy.php / _striptext() line 720 and something?

  • Patch doesn't appear to be accepted format for which Trac can read. I'm not sure why this is, but it would be helpful.

Patch does exploit tracs display, I reported it upstream.

Thanks for the review, please let me know for the other two points. with the encoding of the snoopy class it looks like that those values need to be properly encoded in the PHP code instead of just putting them in. I think the changes happens on my side becaue I checkout the whole project as UTF-8.

Do you have the binary values that are used in there? Maybe that is exploiting the trac display as well? Should we create another ticket to just fix the encoding issue in the snoopy class so we get it ASCII-7BIT save?

#4 in reply to: ↑ 3 ; follow-up: @jacobsantos
9 years ago

Replying to hakre:

Replying to jacobsantos:

Second patch looks good except for a few issues:

  • Added subpackage to PemFTP files, and I suppose to fix the Pure class and the Socket class from not showing up in nightlies. The fix for this is simple without adding subpackage and should be fixed in a new ticket.

I don't understand what that means, but if it's easy to fix, good :)

Well, I was looking at WordPress phpdoc web site and the Pure and the Socket classes weren't showing up. The files were, so I'm guessing that the classes do not have class level phpdoc which is why they aren't showing up. This should be a in a new ticket to fix that.

I actually have no plans to fix it though, I was simply pointing out that it doesn't belong in this ticket and that the fix is relatively simple to do, if not time consuming, but I guess a simple line or two putting the class in the right place would be easy enough.

  • The patch is dirty in some places making changes not related to the @license phpdoc tag. Some examples include the UTF8 translation to ANSI function.

Do you mean character encoding in /wp-includes/class-snoopy.php / _striptext() line 720 and something?

About the character encoding. I'm not sure. I realize that the encoding of the file tends to mess special characters up, but they shouldn't be included in the patch. As it would mess up the file when the patch is applied.

If you are using tortoisesvn, then you could just view changes and then revert the changed characters. I'm not sure what will happen if the encoding hasn't changed. You might have to fix that on your end with a new check out or simply copying the file over checking out the file(s) with the correct encoding.

@hakre
9 years ago

2nd iteration, tagging untagged files. (fixed encoding)

#5 @hakre
9 years ago

I created a new version of the patch with fixed encoding.


Snoopy / Encoding (and I found another file): #14735

#6 in reply to: ↑ 4 @hakre
9 years ago

Replying to jacobsantos:

Well, I was looking at WordPress phpdoc web site and the Pure and the Socket classes weren't showing up. The files were, so I'm guessing that the classes do not have class level phpdoc which is why they aren't showing up. This should be a in a new ticket to fix that.

I can not say why that is, normally I would check PHPDOCs log for issues it's running over.


Everything else should be addressed now in the patch.

#7 @hakre
9 years ago

Related: #14944

#8 @hakre
9 years ago

Related: #14783

#9 @hakre
9 years ago

  • Component changed from General to Inline Docs

#10 @hakre
9 years ago

Refreshed against trunk, patch was stale. Removed the @subpackage tags from the PEMMFTP package files as it is not in the scope of this ticket.

@hakre
9 years ago

@hakre
9 years ago

@hakre
9 years ago

#11 @hakre
9 years ago

Streamlined the patch now. Run over an encoding issue in wp-includes/Text/Diff/Engine/string.php as it contains some ANSI chars (Ö) that got broken with UTF8. Should be okay to commit as I left that file in ANSI encoding.

#12 @hakre
8 years ago

Related: #15585

#13 @lloydbudd
8 years ago

Generally, I'd prefer linking to http://www.opensource.org/licenses/ for all licenses, as the FSF landing pages can be a little political and promotional of their newest versions.

#14 @markjaquith
8 years ago

  • Keywords 3.2-early added
  • Milestone changed from Awaiting Review to Future Release

#15 @nacin
7 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

We should not be modifying external libraries unnecessarily.

Note: See TracTickets for help on using tickets.