Psalm #778

SignpostMarv · 2019-10-01T20:55:29Z

No description provided.

oliverklee · 2019-10-01T21:00:24Z

This will probably greatly benefit from #777.

SignpostMarv · 2019-10-01T21:34:58Z

So there's an issue in that slevomat/coding-standard 5.0.4 requires php 7.1 & there's a sniff used that isn't in the latest version that supports php 7.0.

Rather than tinkering with the travis config, I'd recommend dropping php 7.0, given that it's been EOL for 8 months.

oliverklee · 2019-10-01T21:44:03Z

So there's an issue in that slevomat/coding-standard 5.0.4 requires php 7.1 & there's a sniff used that isn't in the latest version that supports php 7.0.

If that's the case, then we need to revert part of #773. I wonder why the Travis build has not failed, though.

As for the minimum PHP version, we plan to require PHP >= 7.1 for Emogrifier 5.0 (i.e., for the next-after-next breaking release). As we have announced that PHP >= 7.0 will suffice for Emogrifier 4.0.0, I'd like to not break that promise.

(We could release 4.0.0 quite soon so we can drop PHP 7.0, though.)

SignpostMarv · 2019-10-01T21:47:18Z

it's also worth noting that 7.1 goes EOL as of next year.

oliverklee · 2019-10-01T21:52:43Z

I have created #779 to resolve the problem with PHP 7.0.

SignpostMarv · 2019-10-01T22:01:28Z

I'll rebase once that's merged, although possibly not this evening.

oliverklee · 2019-10-01T22:05:41Z

Thanks, that's perfectly fine. There is no hurry.

By the way, I'd prefer to have relatively low-impact PRs, e.g., one for adding psalm to the CI chain (without any code changes) and for adding a baseline, then others for fixing issues found by psalm, another for adding psalm-specific annotations, one for sorting the dependencies etc.

SignpostMarv · 2019-10-01T22:07:18Z

By the way, I'd prefer to have relatively low-impact PRs, e.g.,

split ef33fbf into 3 on the rebase?

oliverklee · 2019-10-01T22:11:30Z

split ef33fbf into 3 on the rebase?

Yes, thanks, that would be great.

composer.json

JakeQZ

I've added a few comments/suggestions/observations. @SignpostMarv, thanks for contributing, looks like a fair bit of work 🤩

composer.json

src/Emogrifier/HtmlProcessor/AbstractHtmlProcessor.php

This commit updates the changelog in reference to the previous commits implementing psalm checking & resolving issues. regarding: MyIntervals#537, MyIntervals#778

SignpostMarv · 2019-10-02T22:22:36Z

This is why I generally commit all psalm bug fixes en-masse.

psalm.baseline.xml

JakeQZ

Looks generally excellent. I am close to being happy for this to be meged to master, but not quite.

There are some changes relating to methods being static vs non-static I'm not totally happy with, because thier counterparts in PHPUnit are static.

There are also a few other tiny improvements that could be made (really minor issues). But I'd be happy for them to be addressed in a separate PR, so that we can get this one merged.

And I think the config files should be in the config directory. I'd be totally happy with this being addressed by a separate future PR – handling changes like this now in this PR I'm sure would be an unnecessary pain 😉

@oliverklee, what do you think?

JakeQZ · 2019-10-02T23:33:07Z

composer.json

@@ -74,6 +76,7 @@
        "ci:php:sniff": "phpcs config src tests",
        "ci:php:fixer": "php-cs-fixer --config=config/php-cs-fixer.php fix --dry-run -v --show-progress=dots --diff-format=udiff config/ src/ tests/",
        "ci:php:md": "phpmd src text config/phpmd.xml",
+        "ci:php:psalm": "psalm --show-info=false",


Could we add this to Travis? If so, let's do it as a separate PR (and get this one merged sooner rather than later).

EDIT: see #736

Had overlooked this, was running composer run ci locally.

There's also vimeo/psalm#1192 to take into account, the fix for which wasn't yet introduced in 3.2 (which you're stuck using because of various dependencies).

OK. Let's do this as a separate PR. I've added #790 so that it is captured and this conversation can be resolved.

Oh, I see you've pushed a commit to this PR adding the step to Travis, but it's currently failing due to some remaining issues. I suggest also addressing the remaining issues as that (or another) separate PR - this one is beginning to become hard to follow and keep track of via the GitHub UI.

psalm.baseline.xml

src/CssInliner.php

src/HtmlProcessor/AbstractHtmlProcessor.php

JakeQZ · 2019-10-03T00:04:37Z

tests/Support/Traits/AssertCss.php

        int $expectedCount,
        string $needle,
        string $haystack
    ) {
-        self::assertSame(
+        $this->assertSame(


assertSame is a static method and should be called staticly. Is there an issue with Psalm that we need a workaround for here?

that'd be NonStaticSelfCall 7d7a528#diff-db03589574d7c5d074966c97658929d9

Looks like an issue with Psalm then. assertSame is definitely a static method in PHPUnit.

I've added #792 to capture this. For now, can we revert changes relating to calls via self:: vs $this (and also any changes making methods static vs non-static), so that we can get this PR merged, and then move on to address the remaining issues separately?

tests/Unit/CssInlinerTest.php

tests/Unit/HtmlProcessor/HtmlPrunerTest.php

oliverklee · 2019-10-03T09:53:19Z

@SignpostMarv Thanks for this huge effort!

I'd like to keep "separate things in separate PRs", i.e., one separate PR for adding psalm to the build and adding the baseline, and then one (or more) separate PRs for fixing things found by psalm and adding psalm-specific annotations. Optimally, those separate PRs will start with one commit each.

oliverklee · 2019-10-03T09:53:49Z

(Smaller PRs will also make the reviews easier and help get the parts merged a lot faster.)

SignpostMarv · 2019-10-03T12:41:35Z

@oliverklee @JakeQZ When I split this into multiple PRs, should the NonStaticSelfCall be split onto it's own PR?

Optimally, those separate PRs will start with one commit each.

At present this would result in 30~ PRs.

oliverklee · 2019-10-03T13:25:07Z

Optimally, those separate PRs will start with one commit each.

Sorry, my comment was a bit ambiguous. I meant "Define well-cut packages for each PR, and if necessary, squash several commits for the same package together before creating the PR for that package."

oliverklee · 2019-10-03T13:29:15Z

When I split this into multiple PRs, should the NonStaticSelfCall be split onto it's own PR?

In principle, yes. (Although I do not agree with that particular change. I'll add a comment to that commit in a second.)

This commit updates the changelog in reference to the previous commits implementing psalm checking & resolving issues. regarding: MyIntervals#537, MyIntervals#778

regarding: MyIntervals#778

JakeQZ · 2019-10-06T13:56:43Z

tests/Support/Traits/AssertCss.php

+                /** @psalm-var array{0:string, 1:string, 2:string, 3:string} */
+                $matches = $matches;
+


Can this instead be @psalm-param for the function/closure?

see commit message for f6ea5a1

JakeQZ · 2019-10-06T14:44:18Z

Everybody, please let's split this into smaller PRs and merge those.

PR by file is probably a good approach, given that I've been going up from the bottom of the baseline.

Can I suggest by specific Psalm check rather than by file?

i.e. Enable one-by-one the checks in <issueHandlers> in psalm.xml and address the code changes required for that specific issue. And each issueHandler in a separate PR. With a CHANGELOG entry for each one in the Fixed section "Issues detected by Psalm {name-of-issue-handler}".

If there are some <issueHandlers> that can be added without making any code changes, then we could add those as a single PR, and probably do not need a CHANGELOG entry since we already have "Check the code with Psalm" and there was nothing else to fix.

@SignpostMarv, @oliverklee, does that make sense?

SignpostMarv · 2019-10-06T14:53:51Z

i.e. Enable one-by-one the checks in <issueHandlers> in psalm.xml and address the code changes required for that specific issue.

issues error by default.

SignpostMarv · 2019-10-06T14:59:50Z

Can I suggest by specific Psalm check rather than by file?

PR by file would mean continuous blocks would be removed from ./psalm.baseline.xml, generally having a single block to merge- making rebasing and merging more straightforward.

PR by type can introduce issues of other types, possibly introducing issues a PR was already made for; there's a couple of those in this PR.

JakeQZ · 2019-10-06T14:59:59Z

issues error by default.

So we could enable one issue, and fix the errors for that specific issue, and have that as a single PR? Rinse and repeat?

Those that cause errors we cannot address right now (e.g. this vs self due to version constraints) will have to wait until PHP 7.0 support is dropped (or we allow the Psalm step to be skipped for PHP 7.0 in Travis, but still run for PHP 7.1+).

SignpostMarv · 2019-10-06T15:06:11Z

So we could enable one issue, and fix the errors for that specific issue, and have that as a single PR? Rinse and repeat?

prepping an explanation on #795 in response to query there.

JakeQZ · 2019-10-06T15:08:02Z

prepping an explanation on #795 in response to query there.

OK, thanks. Just been reading https://erik.booij.me/a-year-with-psalm/

SignpostMarv · 2019-10-06T15:13:43Z

prepping an explanation on #795 in response to query there.

following on from that explanation, what one could do is:

set an issue type to "info",
run psalm with --set-baseline
remove the issue type entry added in step 1
run psalm --show-info=false
fix issues.

what I've been doing is:

remove an issue type from a particular file in the baseline, or remove a single instance if there are many
run psalm --show-info=false
fix issues

JakeQZ · 2019-10-06T15:32:20Z

following on from that explanation

Thanks for that. I'm beginning to get a grasp on how it works 🙂

what one could do is

I'm just wanting to be sure we're taking the best approach to make all our lives easier, both in terms of being able to keep track of what changes are being made to the code base and why, and to minimize the effort actually required to get to a good baseline.

JakeQZ · 2019-10-06T23:32:05Z

...

Really, any way that breaks down the changes into small mangeable chunks would be good :)

@SignpostMarv, we very much appreciate the effort you've been putting in here, it's been amazing.

This commit updates the changelog in reference to the previous commits implementing psalm checking & resolving issues. regarding: MyIntervals#537, MyIntervals#778

regarding MyIntervals#778 (comment)

regarding: MyIntervals#778 (comment)

This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: MyIntervals#794, MyIntervals#778 (comment)

regarding: MyIntervals#778 (comment)

This commit updates the changelog in reference to the previous commits implementing psalm checking & resolving issues. regarding: MyIntervals#537, MyIntervals#778

regarding MyIntervals#778 (comment)

regarding: MyIntervals#778 (comment)

This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: MyIntervals#794, MyIntervals#778 (comment)

This commit updates the changelog in reference to the previous commits implementing psalm checking & resolving issues. regarding: MyIntervals#537, MyIntervals#778

regarding MyIntervals#778 (comment)

regarding: MyIntervals#778 (comment)

This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: MyIntervals#794, MyIntervals#778 (comment)

This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: * MyIntervals#794 * MyIntervals#778 (comment) * MyIntervals#805 (review)

… updating baseline. This commit resolves one of the PossiblyNullPropertyAssignmentValue issues identified in src/HtmlProcessor/AbstractHtmlProcessor.php at the expense of identifying various further Null issues regarding: #537 (Note: the following commit messages from the PR are included for completeness, but may overlap with others due to merges/rebases.) * [BUGFIX] partially satisfy psalm, updating baseline This commit resolves the PossiblyNullReference issues introduced by a previous [BUGFIX] in src/HtmlProcessor/AbstractHtmlProcessor.php by refactoring `$this->domDocument->` to `$this->getDomDocument()->` This necessarily alters the body of the previously identified NullableReturnStatement issue. regarding: #537 * [BUGFIX] partially satisfy psalm, updating baseline This commit resolves 1 InvalidNullableReturnType and 1 NullableReturnStatement issue identified in src/HtmlProcessor/AbstractHtmlProcessor.php by throwing an exception if `AbstractHtmlProcessor::$domDocument` has not been set. regarding: #537 * [BUGFIX] satisfy php-cs-fixer This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: * #794 * #778 (comment) * #805 (review) * [TASK] Add exception code based on UNIX timestamp regarding: #805 (comment) * [CLEANUP] Adjust indentation This commit adjusts the indentation separate from the addition of parenthesis in a previous commit, on the basis of making indentation & line break changes separate from functional changes. regarding: #805 (comment)

* [BUGFIX] resolve PossiblyNull psalm issues via phpunit instanceof assertion, re: #537 This commit resolves the PossiblyNull issue identified in tests/Unit/CssInlinerTest.php by adding an additional assertion that would force the test to fail if the `DOMNodeList::item()` resolved to a non-element result (i.e. null or other DOMNode type) regarding: #537 * [CLEANUP] removing superfluous line breaks regarding: #778 (comment) * [BUGFIX] Partially satisfy psalm, update baseline This commit resolves a set of InvalidOperand issues by using the psalm-specific docblock tags `@psalm-return`, `@psalm-var`, and `@psalm-param` regarding: #801 (review)

This commit updates the changelog in reference to the previous commits implementing psalm checking & resolving issues. regarding: MyIntervals#537, MyIntervals#778

regarding MyIntervals#778 (comment)

This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: MyIntervals#794, MyIntervals#778 (comment)

oliverklee assigned SignpostMarv Oct 1, 2019

oliverklee added cleanup enhancement labels Oct 1, 2019

oliverklee added this to the 4.0.0 milestone Oct 1, 2019

JakeQZ reviewed Oct 2, 2019

View reviewed changes

composer.json Outdated Show resolved Hide resolved

JakeQZ reviewed Oct 2, 2019

View reviewed changes

composer.json Outdated Show resolved Hide resolved

composer.json Outdated Show resolved Hide resolved

src/Emogrifier/HtmlProcessor/AbstractHtmlProcessor.php Outdated Show resolved Hide resolved

SignpostMarv mentioned this pull request Oct 2, 2019

[CLEANUP] adding & utilising composer sort-packages #788

Merged

SignpostMarv force-pushed the psalm branch from 06b0c0c to e950415 Compare October 2, 2019 22:21

SignpostMarv commented Oct 2, 2019

View reviewed changes

psalm.baseline.xml Show resolved Hide resolved

SignpostMarv marked this pull request as ready for review October 2, 2019 22:35

JakeQZ requested changes Oct 3, 2019

View reviewed changes

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 3, 2019

[TASK] run psalm on travis

e79d74a

regarding: MyIntervals#778

SignpostMarv mentioned this pull request Oct 3, 2019

[TASK] Check the code with psalm #789

Merged

JakeQZ reviewed Oct 6, 2019

View reviewed changes

oliverklee mentioned this pull request Oct 6, 2019

Add missing exception codes #797

Closed

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[CLEANUP] set order of checks based on default

102079b

regarding MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[CLEANUP] removing superfluous line breaks

41c1a58

regarding: MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[BUGFIX] satisfy php-cs-fixer

0801e2a

This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: MyIntervals#794, MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[CLEANUP] removing superfluous line breaks

4a28cbc

regarding: MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[CLEANUP] set order of checks based on default

6ea8195

regarding MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[CLEANUP] removing superfluous line breaks

add5254

regarding: MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[BUGFIX] satisfy php-cs-fixer

6a5ae91

This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: MyIntervals#794, MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[CLEANUP] set order of checks based on default

83a4dd5

regarding MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[CLEANUP] removing superfluous line breaks

436d0a4

regarding: MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[BUGFIX] satisfy php-cs-fixer

85382ec

This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: MyIntervals#794, MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[CLEANUP] set order of checks based on default

95b20f3

regarding MyIntervals#778 (comment)

SignpostMarv added a commit to SignpostMarv/emogrifier that referenced this pull request Oct 7, 2019

[BUGFIX] satisfy php-cs-fixer

b8cdcaa

This commit resolves the yoda conditions introduced in previous [BUGFIX] commits. regarding: MyIntervals#794, MyIntervals#778 (comment)

		/** @psalm-var array{0:string, 1:string, 2:string, 3:string} */
		$matches = $matches;

Psalm #778

Psalm #778

Conversation

SignpostMarv commented Oct 1, 2019

oliverklee commented Oct 1, 2019

SignpostMarv commented Oct 1, 2019

oliverklee commented Oct 1, 2019

SignpostMarv commented Oct 1, 2019

oliverklee commented Oct 1, 2019

SignpostMarv commented Oct 1, 2019 • edited Loading

oliverklee commented Oct 1, 2019 • edited Loading

SignpostMarv commented Oct 1, 2019

oliverklee commented Oct 1, 2019

JakeQZ left a comment

Choose a reason for hiding this comment

SignpostMarv commented Oct 2, 2019

JakeQZ left a comment

Choose a reason for hiding this comment

JakeQZ Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee commented Oct 3, 2019

oliverklee commented Oct 3, 2019

SignpostMarv commented Oct 3, 2019

oliverklee commented Oct 3, 2019

oliverklee commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeQZ commented Oct 6, 2019 • edited Loading

SignpostMarv commented Oct 6, 2019

SignpostMarv commented Oct 6, 2019

JakeQZ commented Oct 6, 2019

SignpostMarv commented Oct 6, 2019

JakeQZ commented Oct 6, 2019

SignpostMarv commented Oct 6, 2019

JakeQZ commented Oct 6, 2019

JakeQZ commented Oct 6, 2019

SignpostMarv commented Oct 1, 2019 •

edited

Loading

oliverklee commented Oct 1, 2019 •

edited

Loading

JakeQZ Oct 2, 2019 •

edited

Loading

JakeQZ commented Oct 6, 2019 •

edited

Loading