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.

Sign up for GitHub

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

Jump to bottom

Add template files to checks #2847

Closed
wants to merge 27 commits into from
Closed

Add template files to checks #2847

wants to merge 27 commits into from

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Dec 25, 2022 •
edited
Loading

Description (*)

Add templates files to ...

  • phpstan checks
  • php-cs-fixer checks

Updated

  • docs for template files (may not complete)
  • phpstan baseline (just added ... fixed nothing)

To do

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@sreichel sreichel marked this pull request as draft December 25, 2022 04:06
@github-actions github-actions bot added the Template : install Relates to install template label Dec 25, 2022
@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Page Relates to Mage_Page Template : admin Relates to admin template labels Dec 25, 2022
@sreichel sreichel changed the title Add app/design to checks Add template files to checks Dec 25, 2022
@github-actions github-actions bot added Component: Api PageRelates to Mage_Api Component: Api2 Relates to Mage_Api2 Component: Authorizenet Relates to Mage_Authorizenet Component: Bundle Relates to Mage_Bundle Component: Captcha Relates to Mage_Captcha Component: Catalog Relates to Mage_Catalog Component: CurrencySymbol Relates to Mage_CurrencySymbol Component: Customer Relates to Mage_Customer Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: Media Relates to Mage_Media Component: Newsletter Relates to Mage_Newsletter Component: Oauth Relates to Mage_Oauth Component: Sales Relates to Mage_Sales Component: Tax Relates to Mage_Tax Component: Usa Relates to Mage_Usa Component: Widget Relates to Mage_Widget labels Dec 25, 2022
@github-actions github-actions bot added the Component: CatalogSearch Relates to Mage_CatalogSearch label Dec 25, 2022
@sreichel sreichel marked this pull request as ready for review January 2, 2023 19:16
@sreichel
Copy link
Contributor Author

sreichel commented Jan 2, 2023

@tmotyl thanks :)

tmotyl
tmotyl reviewed Jan 2, 2023
View reviewed changes
app/design/adminhtml/default/default/template/api/rolesedit.phtml
@@ -29,8 +29,8 @@
<span class="field-row">
<label for="all"><?php echo $this->__('Resource Access') ?></label>
<select id="all" name="all" onchange="$('resources_container').toggle()" class="select">
<option value="0" <?php echo ($this->getEverythingAllowed()?'':'selected="selected"'); ?>><?php echo $this->__('Custom') ?></option>
<option value="1" <?php echo ($this->getEverythingAllowed()?'selected="selected"':''); ?>><?php echo $this->__('All') ?></option>
<option value="0" <?php echo($this->getEverythingAllowed() ? '' : 'selected="selected"'); ?>><?php echo $this->__('Custom') ?></option>
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space before echo. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, its not,

Added spaces. Run php-cs-fixer again. Nothing changed. Dont know why space got removed.

Copy link
Contributor Author

@sreichel sreichel Jan 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Last commit added spaces to lines that did not changed before .... so it already was inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems you've found a bug.

running php-cs-fixer ...

  • w/o custom path fixes (remove) that spaces
  • w/ custom path app/design/ finds nothing - wrong, BUT ...
  • w/ custom path app/ finds issues in code/core/Zend - correct, because directory is ignored in config

@tmotyl
Copy link
Contributor

tmotyl commented Jan 2, 2023

This change is super useful, finally to have code completion in templates and phpstan coverage! Thanks a lot.

@sreichel
Copy link
Contributor Author

sreichel commented Jan 2, 2023

Did not check all templates files ... i'm sure there are still some w/o docs (no code completion).

@sreichel sreichel marked this pull request as draft January 2, 2023 21:38
@sreichel
Copy link
Contributor Author

sreichel commented Jan 2, 2023 •
edited
Loading

grep -riL --include \*.phtml "@var" app/design/ | wc -l

756 files with missing docsblock for $this.

(need seperate PR)

@sreichel sreichel marked this pull request as ready for review January 2, 2023 21:55
@sreichel sreichel marked this pull request as draft January 2, 2023 22:20
@sreichel sreichel marked this pull request as ready for review January 2, 2023 22:47
@sreichel sreichel closed this by deleting the head repository Jan 8, 2023
@addison74 addison74 reopened this Jan 9, 2023
@sreichel
Copy link
Contributor Author

@addison74 nice try, but does not work. ;)

(I'll re-add one or two PRs ... everthing else later)

@sreichel sreichel closed this Jan 10, 2023
@sreichel sreichel mentioned this pull request Jan 15, 2023
Closed
4 tasks
@sreichel sreichel mentioned this pull request Feb 20, 2024
Open
32 tasks
@sreichel sreichel mentioned this pull request Oct 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmotyl tmotyl tmotyl left review comments

Assignees
No one assigned
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Api PageRelates to Mage_Api Component: Api2 Relates to Mage_Api2 Component: Authorizenet Relates to Mage_Authorizenet Component: Bundle Relates to Mage_Bundle Component: Captcha Relates to Mage_Captcha Component: Catalog Relates to Mage_Catalog Component: CatalogInventory Relates to Mage_CatalogInventory Component: CatalogSearch Relates to Mage_CatalogSearch Component: Checkout Relates to Mage_Checkout Component: Cms Relates to Mage_Cms Component: Core Relates to Mage_Core Component: CurrencySymbol Relates to Mage_CurrencySymbol Component: Customer Relates to Mage_Customer Component: Directory Relates to Mage_Directory Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: Giftmessage Relates to Mage_Giftmessage Component: GoogleAnalytics Relates to Mage_GoogleAnalytics Component: Index Relates to Mage_Index Component: Media Relates to Mage_Media Component: Newsletter Relates to Mage_Newsletter Component: Oauth Relates to Mage_Oauth Component: Page Relates to Mage_Page Component: Payment Relates to Mage_Payment Component: PayPal Relates to Mage_Paypal Component: Persistant Relates to Mage_Persistant Component: Poll Relates to Mage_Poll Component: ProductAlert Relates to Mage_ProductAlert Component: Rating Relates to Mage_Rating Component: Reports Relates to Mage_Reports Component: Review Relates to Mage_Review Component: Rss Relates to Mage_Rss Component: Sales Relates to Mage_Sales Component: Sendfriend Relates to Mage_Sendfriend Component: Shipping Relates to Mage_Shipping Component: Tag Relates to Mage_Tag Component: Tax Relates to Mage_Tax Component: Usa Relates to Mage_Usa Component: Widget Relates to Mage_Widget Component: Wishlist Relates to Mage_Wishlist composer Relates to composer.json environment php-cs-fixer phpstan Template : admin Relates to admin template Template : base Relates to base template Template : default Relates to base template Template : install Relates to install template Template : rwd Relates to rwd template
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sreichel @addison74 @tmotyl