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

Omitting Script attribute if none or default #53

Merged
merged 1 commit into from
Oct 25, 2020

Conversation

DennisDobslaf
Copy link
Contributor

@DennisDobslaf DennisDobslaf commented Aug 11, 2020

Ommiting the script attribute if its either empty or default (text/javascript) and the doctype is set to HTML5.
Referring to: https://html.spec.whatwg.org/multipage/scripting.html#the-script-element

Signed-off-by: Dennis Dobslaf [email protected]

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

This is great - I just have a couple nit-picks for you to address, and then I can merge.

Thanks!

src/Helper/HeadScript.php Show resolved Hide resolved
test/Helper/HeadScriptTest.php Outdated Show resolved Hide resolved
@DennisDobslaf
Copy link
Contributor Author

@weierophinney The CI failed because of the required PHP Version (private const needs PHP 7.1). If we change this, we cannot require PHP 5.6 anymore. Wouldn't this be a BC Break?

@michalbundyra
Copy link
Member

@DennisDobslaf we can bump PHP version to 7.2 for the next minor, please see our policy here:
https://github.com/laminas/technical-steering-committee/blob/main/meetings/minutes/2020-03-02-TSC-Minutes.md#lts-policy

We are not considering bumping PHP version in minor update as a BC break.

@DennisDobslaf
Copy link
Contributor Author

@DennisDobslaf we can bump PHP version to 7.2 for the next minor, please see our policy here:
https://github.com/laminas/technical-steering-committee/blob/main/meetings/minutes/2020-03-02-TSC-Minutes.md#lts-policy

We are not considering bumping PHP version in minor update as a BC break.

Well, but this is not a minor update but a fix, isn't it?

So, should it be minor update with private const plus PHP 7.2 requirement
or
a fix without private const and leaving the PHP Version as it is?

Should this be a discussion in the slack channel?

@samsonasik
Copy link
Member

php 5.6, 7, and 7.1 can be removed from .travis.yml then

@DennisDobslaf
Copy link
Contributor Author

Any work to do for me now?

@froschdesign froschdesign linked an issue Aug 18, 2020 that may be closed by this pull request
@froschdesign froschdesign added the Bug Something isn't working label Aug 18, 2020
@froschdesign
Copy link
Member

I think there are some misunderstandings. This is a bug fix and the fix should be released with a mini release.
The bump of the PHP version should done separated and within a new minor version. (Btw. the version 7.2 of PHP is outdated and will also be dead in 3 months, so it should be dropped.)

This allows us to make a release now and not to wait for the next minor version.


Which means for this pull request:

  • remove the visibility modifier from class constant Laminas\View\Helper\HeadScript\DEFAULT_SCRIPT_TYPE
  • undo all changes in Composer configuration
  • undo all changes in Travis configuration
  • squashing commits

@DennisDobslaf
Copy link
Contributor Author

I think there are some misunderstandings. This is a bug fix and the fix should be released with a mini release.
The bump of the PHP version should done separated and within a new minor version. (Btw. the version 7.2 of PHP is outdated and will also be dead in 3 months, so it should be dropped.)

This allows us to make a release now and not to wait for the next minor version.

Which means for this pull request:

  • remove the visibility modifier from class constant Laminas\View\Helper\HeadScript\DEFAULT_SCRIPT_TYPE
  • undo all changes in Composer configuration
  • undo all changes in Travis configuration
  • squashing commits

I'll do

@froschdesign
Copy link
Member

@DennisDobslaf
Thank you for your understanding and apologies for the trouble!

@froschdesign froschdesign added this to the 2.11.5 milestone Aug 18, 2020
@DennisDobslaf
Copy link
Contributor Author

DennisDobslaf commented Aug 18, 2020

@froschdesign I'm fine with your notes.
I changed, squashed and pushed now.

@@ -33,6 +33,7 @@ class HeadScript extends Placeholder\Container\AbstractStandalone
*/
const FILE = 'FILE';
const SCRIPT = 'SCRIPT';
const DEFAULT_SCRIPT_TYPE = 'text/javascript';
Copy link
Member

Choose a reason for hiding this comment

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

This constant should also be used in Laminas\View\Helper\InlineScript:

public function __invoke(
$mode = self::FILE,
$spec = null,
$placement = 'APPEND',
array $attrs = [],
$type = 'text/javascript'
) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like $type = HeadScript::DEFAULT_SCRIPT_TYPE
or by making a separated "Enum" Class?

Copy link
Member

Choose a reason for hiding this comment

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

The InlineScript helper is an extension of the HeadScript helper:

class InlineScript extends HeadScript

Therefore: $type = self::DEFAULT_SCRIPT_TYPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I've overlooked this fact.

Copy link
Member

Choose a reason for hiding this comment

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

Probably, as we cannot add visibility on the constant, we should add php annotation @internal for now. Then in next minor, when we bump php version we can use private/protected visibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added your suggestion @michalbundyra

@DennisDobslaf DennisDobslaf force-pushed the omit-type-for-script-tag branch 2 times, most recently from a1809e7 to cbef6ab Compare August 18, 2020 13:24
@DennisDobslaf
Copy link
Contributor Author

DennisDobslaf commented Aug 18, 2020 via email

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

One final change, and I think this is ready!

test/Helper/HeadScriptTest.php Outdated Show resolved Hide resolved
Ommiting the script attribute if its either empty or default (text/javascript) and the doctype is set to HTML5.
Referring to: https://html.spec.whatwg.org/multipage/scripting.html#the-script-element

Signed-off-by: Dennis Dobslaf <[email protected]>
@samsonasik
Copy link
Member

@weierophinney I think it is ready now

@DennisDobslaf
Copy link
Contributor Author

Any news on this one? Do I have to do some work?

@geerteltink geerteltink changed the base branch from master to 2.11.x September 11, 2020 12:02
@samsonasik
Copy link
Member

ping @weierophinney

@froschdesign
Copy link
Member

@samsonasik
You can merge it yourself! No need to ping the project leader here. 😉

@samsonasik samsonasik merged commit 81103d2 into laminas:2.11.x Oct 25, 2020
@samsonasik
Copy link
Member

Merged 🎉 @DennisDobslaf thank you for the PR ;)
@froschdesign thank you for the review ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

type in script tags for classic scripts (js) should be ommited
5 participants