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

Update closing HTML tags in form_helper.php #6658

Conversation

visserrobin
Copy link

Description
Fixes #6649
Removed the '/' in closing tags for input fields for text, upload and checkbox.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@visserrobin visserrobin linked an issue Oct 10, 2022 that may be closed by this pull request
@ddevsr
Copy link
Collaborator

ddevsr commented Oct 10, 2022

Dont forget changes unit tests for this PR

@kenjis kenjis added the wrong branch PRs sent to wrong branch label Oct 10, 2022
@kenjis
Copy link
Member

kenjis commented Oct 10, 2022

Sorry, this is not a bug fix, so please send it to 4.3 not develop.
See https://github.com/codeigniter4/CodeIgniter4/blob/caa355d78bc2eb0029842ee3765d6dda23f95f71/contributing/workflow.md#if-you-sent-to-the-wrong-branch

@kenjis kenjis changed the title Update form_helper.php Update closing HTML tags in form_helper.php Oct 10, 2022
@neznaika0
Copy link
Contributor

neznaika0 commented Oct 11, 2022

Please remove the space at the end.
The output will be:
<input type="text" > to <input type="text">

@kenjis kenjis added the GPG-Signing needed Pull requests that need GPG-Signing label Oct 11, 2022
@kenjis
Copy link
Member

kenjis commented Oct 11, 2022

@sclubricants
Copy link
Member

@kenjis

Im not sure that removing the closing tag is the best way to go here. The closing tag is needed for the more stricter XHTML. XHTML requires all elements to be closed including void elements. The closing tag on a plain HTML document is still valid even if some validation says its not. It is simply ignored. By omitting these you are causing problems for those who want to use XHTML markup. The presence of them has no affect on plain HTML markup.

If you really have to get rid of them then I would have a config definition to decide how to render the tags.

@kenjis
Copy link
Member

kenjis commented Oct 17, 2022

I think it is better to follow the standard.
https://html.spec.whatwg.org/

XHTML 1.1 specification is Superseded Recommendation.

@kenjis
Copy link
Member

kenjis commented Oct 17, 2022

@sclubricants

If you really have to get rid of them then I would have a config definition to decide how to render the tags.

Do you need to support XHTML in year 2022?

@sclubricants
Copy link
Member

I don't use it with codeigniter but I do in Salesforce. Just saying some people might be using it and if you change it it could cause problems.

@sclubricants
Copy link
Member

I at least think this is a breaking change.

@ddevsr
Copy link
Collaborator

ddevsr commented Oct 18, 2022

No activity by author, i can send PR at home later

@ddevsr
Copy link
Collaborator

ddevsr commented Oct 18, 2022

One of criteria SEO is data structure schema must be valid,

Google said on https://developers.google.com/search/docs/appearance/structured-data

We removed Google-specific validation from the Structured Data Testing Tool and migrated the tool to a new domain, Schema Markup Validator (https://validator.schema.org/).

And now based on HTML5, we must support standard structure.

@ddevsr
Copy link
Collaborator

ddevsr commented Oct 18, 2022

@kenjis I have problem in my env with PHPStan 1.8.10

------ ---------------------------------------------------------------------------------------------------------------------------- 
  Line   system\Helpers\form_helper.php
 ------ ----------------------------------------------------------------------------------------------------------------------------      
         Ignored error pattern #^Offset 'checked' on array\{type\: 'checkbox', name\: mixed, value\: string\} in isset\(\) does not       
         exist\.$# in path D:\Project\laragon\www\ci4form\system\Helpers\form_helper.php was not matched in reported errors.
  366    Offset 'checked' on array{type: 'checkbox', name: string, value: string} in isset() does not exist.
 ------ ----------------------------------------------------------------------------------------------------------------------------      


 [ERROR] Found 2 errors

Fix the phpstan error(s) before commit.
    function form_checkbox($data = '', string $value = '', bool $checked = false, $extra = ''): string
    {
        $defaults = [
            'type'  => 'checkbox',
            'name'  => (! is_array($data) ? $data : ''),
            'value' => $value,
        ];

        if (is_array($data) && array_key_exists('checked', $data)) {
            $checked = $data['checked'];
            if ($checked === false) {
                unset($data['checked']);
            } else {
                $data['checked'] = 'checked';
            }
        }

        if ($checked === true) {
            $defaults['checked'] = 'checked';
        } elseif (isset($defaults['checked'])) {
            unset($defaults['checked']);
        }

        return '<input ' . parse_form_attributes($data, $defaults) . stringify_attributes($extra) . ">\n";
    }

@kenjis
Copy link
Member

kenjis commented Oct 18, 2022

@ddevsr I cannot reproduce.
Can you send a PR?

@ddevsr
Copy link
Collaborator

ddevsr commented Oct 19, 2022

@ddevsr I cannot reproduce. Can you send a PR?

Okay, i will send PR with PHPStan 1.8.8

@kenjis
Copy link
Member

kenjis commented Oct 20, 2022

See #6717

@kenjis kenjis closed this Oct 20, 2022
@kenjis kenjis deleted the 6649-dev-remove-self-closing-html-tags-in-helpers-1 branch October 20, 2022 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GPG-Signing needed Pull requests that need GPG-Signing wrong branch PRs sent to wrong branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev: remove self-closing HTML tags in helpers
5 participants