Skip to content

Conversation

@peterfabian
Copy link
Contributor

@peterfabian peterfabian commented Jul 18, 2019

As Class_Moved::$declaraion is not defined, it generates a lot of notices and does not work correctly. I'm guessing the invociation should check for $old_declaration.

Changes proposed in this Pull Request:

Running the Analyzer tool, I was getting a lot of
Notice: Undefined property: Automattic\Jetpack\Analyzer\Differences\Class_Moved::$declaration in .../jetpack/packages/analyzer/src/Differences/Class_Moved.php on line 32. My guess is that the check should happen against old_declaration.

Testing instructions:

When running the compat test for WC core, I noticed this problem. Details of setup along with scripts to run are available in this p2 post: pb22l9-1D-p2

Proposed changelog entry for your changes:

  • Fixed variable name in Analyzer module Class_Moved.

As declaraion is not defined, it generates a lot of notices and does not work correctly. I'm guessing the invociation should check for old_declaration.
@peterfabian peterfabian requested a review from a team July 18, 2019 15:10
@jetpackbot
Copy link
Collaborator

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 8942f1e

@jeherve jeherve added [Status] Needs Review This PR is ready for review. [Type] Bug When a feature is broken and / or not performing as intended [Focus] Jetpack DNA labels Jul 18, 2019
@jeherve jeherve added this to the 7.6 milestone Jul 18, 2019
Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Can confirm that this fixes the warning, thank you!

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jul 25, 2019
@jeherve jeherve merged commit 8b58239 into master Jul 25, 2019
@jeherve jeherve deleted the fix/analyzer-Class_Moved-var-name-fix branch July 25, 2019 17:55
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 25, 2019
emilyatmobtown pushed a commit to emilyatmobtown/jetpack that referenced this pull request Jul 26, 2019
As declaraion is not defined, it generates a lot of notices and does not work correctly. I'm guessing the invociation should check for old_declaration.
jeherve added a commit that referenced this pull request Jul 29, 2019
jeherve added a commit that referenced this pull request Jul 30, 2019
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
jeherve added a commit that referenced this pull request Jul 30, 2019
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Focus] Jetpack DNA [Type] Bug When a feature is broken and / or not performing as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants