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

Switch to using PHP namespaces #401

Merged
merged 56 commits into from
Dec 8, 2020
Merged

Switch to using PHP namespaces #401

merged 56 commits into from
Dec 8, 2020

Conversation

joehoyle
Copy link
Member

To keep inline with the HM coding standards and consistency with other Human Made code, I'd like to swich S3 Uploads to namespaces, and fix up any outstanding PHP formatting issues with the HM PHPCS.

To keep inline with the HM coding standards and consistency with other Human Made code, I'd like to swich S3 Uploads to namespaces, and fix up any outstanding PHP formatting issues with the HM PHPCS.
Copy link

@hm-linter hm-linter bot left a comment

Choose a reason for hiding this comment

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

Linting failed (125 errors).

(36 notices occurred in your codebase, but were on files/lines not included in this PR.)

Rather than needing to install PHP, all modules, imagemagick, and S3 APis locally, docker would be much metter suited to running out tests. This way we can run an S3 replacement (minio) instead of requiring a live integration with S3. This is better for performance and isolation of tests. It also means we have a clearly defined, consistent envirinment for tests for PHP configuration.
@rmccue
Copy link
Member

rmccue commented Apr 29, 2020

#WildIdeas

@joehoyle
Copy link
Member Author

joehoyle commented Jul 8, 2020

@rmccue I have create a v2-branch off master which we can backport things to v2 as needed. By merging this into master, the next major release will be 3.0, I'd like to get this merged in soon.

@joehoyle
Copy link
Member Author

Ping @rmccue for a review on this.

@joehoyle
Copy link
Member Author

Ping @rmccue

Copy link
Member

@rmccue rmccue left a comment

Choose a reason for hiding this comment

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

Some minor fixes necessary, but substantially good. I'll review on a tighter timeline next time around :)

inc/class-plugin.php Show resolved Hide resolved
inc/class-plugin.php Show resolved Hide resolved
inc/class-plugin.php Outdated Show resolved Hide resolved
inc/class-stream-wrapper.php Outdated Show resolved Hide resolved
inc/class-stream-wrapper.php Outdated Show resolved Hide resolved
inc/class-stream-wrapper.php Outdated Show resolved Hide resolved
joehoyle and others added 3 commits December 8, 2020 08:35
@joehoyle
Copy link
Member Author

joehoyle commented Dec 8, 2020

@rmccue ok ready for re-review

@joehoyle joehoyle requested a review from rmccue December 8, 2020 14:21
inc/class-stream-wrapper.php Outdated Show resolved Hide resolved
inc/class-stream-wrapper.php Outdated Show resolved Hide resolved
joehoyle and others added 2 commits December 8, 2020 09:25
Co-authored-by: Ryan McCue <me@ryanmccue.info>
Co-authored-by: Ryan McCue <me@ryanmccue.info>
@joehoyle joehoyle merged commit 9621685 into master Dec 8, 2020
@joehoyle joehoyle deleted the namespaces branch December 8, 2020 14:26
@goldenapples
Copy link
Contributor

I'm wondering - can we get a new version cut on the v2 branch right before this change was made? This pull request breaks compatability with altis/cloud, and I'd like to use a tagged release which includes the Private Attachments functionality from #398 along with altis/cloud v5 or 6.

I think tagging 539d0c1 as 2.3.0 makes sense, and then some of the more recent features can be backported to the v2 branch while master is tracking the future v3 branch.

@joehoyle
Copy link
Member Author

@goldenapples we can back port any fixes to v2 -- however if private attachments wasn't in v2 (sorry can't remember if it was) I don't think we should back port such a large feature that would be a lot of work.

@joehoyle
Copy link
Member Author

@goldenapples it could be that we merged private-attachments before this PR, if that's the case, perhaps we could tag a release, though I'd rather do it as a beta/RC as I don't think we'd want to support that as "working". If your project is already using the private-attachments branch instead, it might be best to just back port your fix to that branch.

@goldenapples
Copy link
Contributor

No, private attachments wasn't in v2. I can definitely go on using the feature branch (and I've already backported a few fixes to it). Just wanted to check on any release plans and see if it made any sense to do a release containing it.

As long as the private-attachments branch doesn't accidentally get cleaned up, I'm fine continuing to track it for now.

@joehoyle
Copy link
Member Author

Ok cool -- I'll also open a PR on Altis for the next major version to upgrade to S3 Uploads v3 too

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.

3 participants