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

Bump VariableAnalysis to ^2.9.0 to allow use of the backported options #588

Closed
GaryJones opened this issue Oct 12, 2020 · 5 comments
Closed

Comments

@GaryJones
Copy link
Contributor

What problem would the enhancement address for VIP?

When template partials are require'd from within a class, then the partial may use $this-> to insert display a value. At PHP runtime, it works, but for PHPCS analysis, it sees a PHP file with an undefined $this and so reports it via the VariableAnalysis package. Technically it's correct, and while it could be ignored, it's not practical or desirable to do that.

Describe the solution you'd like

VariableAnalysis 2.9.0 includes two new options that can control whether these violations are raised. I'd like to have VIPCS require this update in package, so to be able to make use of these options in the default configuration of VIPCS.

VIPCS currently requires ^2.8.3, so we'd need to bump the minimum version.

What code should be reported as a violation?

What code should not be reported as a violation?

Don't report on $this:

<?php
// Example foo.php template.

if ( ! empty( $this->vars['video-id'] ) ) {
	echo '<video...></video>';
}

Don't report on $args (new feature in WP 5.5):

<?php
// Example foo.php template.
 
// Set defaults.
$args = wp_parse_args(
    $args,
    array(
        'class'          => '',
        'arbitrary_data' => array(
            'foo' => 'fooval',
            'bar' => false,
        ),
        ...
    )
);
?>
 
<div class="widget <?php echo esc_html_class( $args['class'] ); ?>">
    <?php echo esc_html( $args['arbitrary_data']['foo'] ); ?>
</div>
@GaryJones
Copy link
Contributor Author

GaryJones commented Mar 7, 2023

Note that the latest VariableAnalysis is 2.11.10, and it includes a lot of the 3.x benefits backported into 2.x.

Edit: 2.11.1 was made the minimum version of VA in VIPCS 2.3.3, so this just needs configuring.

@jrfnl
Copy link
Collaborator

jrfnl commented Mar 7, 2023

Ref: #691

@jrfnl jrfnl modified the milestones: 2.4.0, 2.3.4 Aug 21, 2023
@jrfnl
Copy link
Collaborator

jrfnl commented Aug 22, 2023

I've just been looking at this, but this ticket looks like a duplicate of Automattic/vip-go-ci#120 (different repo), which was fixed by a combination of PR #690 (VA configuration) and #691 (VA version bump), both of which were included in the 2.3.3 release.

I have a feeling this ticket should be closed as fixed in 2.3.3. What am I missing ?

@GaryJones
Copy link
Contributor Author

I agree - this looks like it was handled with later PRs (which just didn't mark this one as fixed).

@GaryJones GaryJones removed this from the 2.3.4 milestone Aug 22, 2023
@GaryJones
Copy link
Contributor Author

Marking as a duplicate, since #691 is already associated with the 2.3.3 milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants