Skip to content

Conversation

matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Apr 19, 2023

This adds a "Getting Started" tutorial article. Right now it is at best half-written, with an emphasis on commands to run to set up the prerequisite tools.

The project readme will probably need to change as a result of writing this article, but I haven't thought about that yet.

Developer

Secrets

  • No secrets are affected

Documentation

  • Project documentation has been updated

Accessibility

  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)

Stakeholder approval

  • Stakeholder approval is not needed

Dependencies

NO dependencies are updated

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • The changes have been verified
  • The documentation has been updated or is unnecessary
  • New dependencies are appropriate or there were no changes

$ brew install composer
```

The only global package currently needed is PHP CodeSniffer, and the WordPress
Copy link
Member

Choose a reason for hiding this comment

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

I never did any of these global phpcs steps. Maybe we add that to an advanced setup doc and focus this on the core seeing as how I never did this and didn't notice for this long lol?

Copy link
Member

Choose a reason for hiding this comment

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

I'm 47% sure my composer security still works as expected but I may need help confirming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... let me identify a step to confirm whether composer security does something. What happens now when you type that command?

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, this is what I see when I run the command at the moment:

% composer security
> phpcs --standard=phpcs.security.xml

Copy link
Member Author

Choose a reason for hiding this comment

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

So, one way to confirm that Composer is successfully running the check:

  1. Find any instance of phpcs:disable in the codebase, and remove that line. One example is this: https://github.com/MITLibraries/mitlib-wp-network/blob/master/web/app/themes/mitlib-child/functions.php#L214
  2. With that line removed, there is now something for the security check to find. This should now be the output when running composer security
% composer security
> phpcs --standard=phpcs.security.xml

FILE: /Users/mjbernha/lando/mitlib-wp-network/web/app/themes/mitlib-child/functions.php
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 214 | ERROR | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found '$output'.
     |       | (WordPress.Security.EscapeOutput.OutputNotEscaped)
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 1.39 secs; Memory: 10MB

Script phpcs --standard=phpcs.security.xml handling the security event returned with error code 1

Copy link
Member Author

Choose a reason for hiding this comment

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

In terms of this PR, if your setup works as described above with composer security, then I think I'm fine with moving this section to a "further setup" document or something? Does that sound good to you?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that works the same for me after removing the two lines from the child themes functions.php.

[I] ⋊> ~/p/mitlib-wp-network on hours  composer security                                                                                                
> phpcs --standard=phpcs.security.xml

FILE: /Users/jprevost/projects/mitlib-wp-network/web/app/themes/mitlib-child/functions.php
----------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------------------------------------------------------
 214 | ERROR | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found '$output'.
     |       | (WordPress.Security.EscapeOutput.OutputNotEscaped)
----------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 2.35 secs; Memory: 10MB

** Why are these changes being introduced:

* It turns out that part of the setup which I'd included in the Getting
  Started tutorial may actually not be needed to get started.

** Relevant ticket(s):

n/a

** How does this address that need:

* This moves the extra bits about setting up PHP CodeSniffer locally,
  and the WordPress coding standards, to a separate tutorial with a more
  advanced focus.

** Document any side effects to this change:

* Hopefully none
@matt-bernhardt
Copy link
Member Author

Okay @JPrevost - I've split that section out to a separate tutorial, leaving behind a more minimal Composer setup step. Does this look better / more like what you followed?

Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

I think this looks good. I'm sure we'll adjust as Adam goes through the process but having a place to point at and say "this part is confusing" will be great. Thanks!

We currently use the latest release on the 8.0.x branch, using commands such as:

```bash
$ brew install php@8.0
Copy link
Member

Choose a reason for hiding this comment

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

php --version                                                                                                    15:31:19
PHP 8.2.4 (cli) (built: Mar 16 2023 16:25:32) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.4, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.4, Copyright (c), by Zend Technologies

uh... oops

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully that bodes well for when we get around to upgrading PHP versions :-)

@matt-bernhardt matt-bernhardt merged commit da21fb0 into master Apr 24, 2023
@matt-bernhardt matt-bernhardt deleted the getting-started branch April 24, 2023 19:42
matt-bernhardt pushed a commit that referenced this pull request Jul 7, 2023
disable revisions by setting false or 0
roots/bedrock#658
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants