Skip to content

Conversation

@johnstevenson
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -->
License MIT
Doc PR

This PR makes the code clearer when only installing and removes extraneous work if the phpunit symlink/junction just needs updating. It additionally fixes a couple of inconsistencies.

This is the first of a few of other minor fixes I would like to propose, which have been split up to be merge-upable friendly.

@nicolas-grekas
Copy link
Member

Hi, thanks for the PR. From the description, this doesn't fix a bug but is a code refactorization, right? This should target 6.1 if confirmed.
Otherwise, please describe the bug.

@GromNaN
Copy link
Member

GromNaN commented Dec 15, 2021

If we are to refactor this code (and target 6.1), we could use a class (with a basic require_once). It would be testable and certainly more readable.

@johnstevenson
Copy link
Contributor Author

@nicolas-grekas Yeah, I was probably stretching the bug bit: it was more about improving performance, though there is an inconsistency in $getEnvVar not returning the default when the env is an emtpy string (only fixed for the the case that caused an issue) - which would have been my second improvement.

My overall plan was to simply get the code into a more readable state for the supported versions so that:

  1. it is easier for folks to make fixes in the future
  2. it is easier to refactor it into a class (in the future).

I'll do whatever you suggest.

@nicolas-grekas
Copy link
Member

Note that refactoring for the sake of it might not be worth it. We have a track record of refactorings that break things so we're quite prudent now :)

@johnstevenson
Copy link
Contributor Author

I've put all the changes I wish to make as a separate PR: #44668

@fancyweb
Copy link
Contributor

fancyweb commented Jan 3, 2022

@johnstevenson Can we close this one in favor of #44668? 🤔

@johnstevenson
Copy link
Contributor Author

Can we close this one in favor of #44668?

I didn't get a specific answer here, so I created that PR to show the full extent of the changes I wished to make.

So I guess someone needs to decide if it would be cleaner to incorporate these fixes/changes on 4.4, or whether they should only be made on 6.1.

@nicolas-grekas
Copy link
Member

We won't merge a refactor on 4.4, so closing here.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants