Skip to content

Changed <a name="..."> to <a id="..."> #416

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

Merged
merged 11 commits into from
Jul 26, 2021
Merged

Conversation

mikeschinkel
Copy link
Contributor

@mikeschinkel mikeschinkel commented Jul 20, 2021

When opening this repo in PhpStorm it flagged @name as a deprecated HTML attribute, so it seemed appropriate to change then to @id. In cases where there were both a @name and @id attribute and their values were the same, this commit removes the @name attribute (there were no cases where both @name and @id attributes existed but having different values.)

Previously the page stated "PHP 4, PHP 5 and PHP 7 are distributed..." even though PHP 8 has been released. This commit changes the wording to be "Starting with PHP 4, versions of the PHP software are distributed..."
PhpStorm flagged @name as a deprecated HTML attribute, so it seemed appropriate to change then to @id. In cases where there were both a @name and @id attribute and their values were the same, this commit removes the @name attribute (there were no cases where both @name and @id attributes existed but having different values.)
@mikeschinkel mikeschinkel changed the title Id vs name Changed <a name="..."> to <a id="..."> Jul 20, 2021
@cmb69
Copy link
Member

cmb69 commented Jul 20, 2021

Thank you! It might be more important, though, to also fix that in the scripts which are used to generate these entries, e.g.

$tag = "<a name=\"PHP_{$parts[0]}_{$parts[1]}\"></a>";

@mikeschinkel
Copy link
Contributor Author

mikeschinkel commented Jul 20, 2021

@cmb69 — lol! I didn't know that, but know I do! :-)

I will fix there and updated.

BTW, I also have a few other PRs are will add, each of which is very narrow so it will be easier to review.

@mikeschinkel
Copy link
Contributor Author

@cmb69 — Where are these files? I can't find them in my cloned repo?

@cmb69
Copy link
Member

cmb69 commented Jul 20, 2021

One is bin/news2html. I think there is at least one other to generate the archive entries, but I'm not sure where that is (might be included/used from bin/createNewsEntry).

@mikeschinkel
Copy link
Contributor Author

@cmb69 — Sorry, I just had a brain fart.

I was looking for the /f16e2cb6a3b0ea30a2c6a585180c20f12f941e82/ directory. Obviously it wasn't there, as that was a commit hash. Doh!

PhpStorm flagged @name as a deprecated HTML attribute, so it seemed appropriate to change then to @id. In cases where there were both a @name and @id attribute and their values were the same, this commit removes the @name attribute (there were no cases where both @name and @id attributes existed but having different values.)
@mikeschinkel
Copy link
Contributor Author

@cmb69 — PR updated to include news2html.php.

One additional question.

PhpStorm flagged line 72 as an error because of the \1:

array('<?php bugfix(\1); ?>', '<?php peclbugfix(\1); ?>', 
      '<?php implemented(\1); ?>', '<?php githubissuel(\'php/php-src\',\1); ?>'),

Is that valid in some earlier version of PHP that you might still be using? Shouldn't it be \$1 instead of just \1?:

array('<?php bugfix(\$1); ?>', '<?php peclbugfix(\$1); ?>', 
      '<?php implemented(\$1); ?>', '<?php githubissuel(\'php/php-src\',\$1); ?>'),

If yes I can either add that fix to this PR or create a new PR; whichever your preference.

@cmb69
Copy link
Member

cmb69 commented Jul 20, 2021

PhpStorm flagged line 72 as an error because of the \1:

Oh, right, this should more correctly be \\1 or even better $1. I'd prefer a separate PR.

HTML deprecated table element attributes @cellpadding, @cellspacing, @border, @align, etc. so this PR replaces them with CSS classes, selectors and style rules.  It also removed a CSS style rule that was being overwritten by the next line, it removes an invalid font rule (See https://stackoverflow.com/a/20641079/102699), it removes redundant `px` from `0px` and it sneaks in a <label> element to wrap an <input> element per PhpStorm's code inspection instructions.
@mikeschinkel
Copy link
Contributor Author

@cmb69 — I believe this PR has addressed the issues you mention and should be ready to merge,

I created two other PRs to deal with the regex issue (#423) and a fix to allow news2html to process the only example NEWS files I could find in the repo (#417.)

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Except for removing the table attributes this looks very good to me.

@cmb69 cmb69 merged commit bdfa964 into php:master Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants