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

PHP8.1 compatibility #2

Merged
merged 6 commits into from
Apr 1, 2022
Merged

PHP8.1 compatibility #2

merged 6 commits into from
Apr 1, 2022

Conversation

eboye
Copy link
Contributor

@eboye eboye commented Feb 25, 2022

Description

I've bumped the php version to 8.1 and added some strict types

Motivation and context

Future versions compatibility

How has this been tested?

unit tests included in the project

Screenshots (if appropriate)

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING document.
  • My pull request addresses exactly one patch/feature.
  • I have created a branch for this patch/feature.
  • Each individual commit in the pull request is meaningful.
  • I have added tests to cover my changes.
  • If my change requires a change to the documentation, I have updated it accordingly.

@yusufkandemir yusufkandemir linked an issue Feb 25, 2022 that may be closed by this pull request
Copy link
Owner

@yusufkandemir yusufkandemir left a comment

Choose a reason for hiding this comment

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

Looking great 👏 I requested some changes to improve clarity.

src/MicrodataDOMDocument.php Outdated Show resolved Hide resolved
src/MicrodataDOMDocument.php Outdated Show resolved Hide resolved
src/MicrodataParser.php Outdated Show resolved Hide resolved
tests/MicrodataParserTest.php Outdated Show resolved Hide resolved
src/MicrodataDOMDocument.php Outdated Show resolved Hide resolved
tests/MicrodataParserTest.php Outdated Show resolved Hide resolved
tests/MicrodataTest.php Outdated Show resolved Hide resolved
src/MicrodataDOMElement.php Outdated Show resolved Hide resolved
src/MicrodataDOMElement.php Outdated Show resolved Hide resolved
src/MicrodataParser.php Outdated Show resolved Hide resolved
@eboye
Copy link
Contributor Author

eboye commented Feb 26, 2022

Sure, no specific reasons why I used those, just a matter of habit adding those :)

@yusufkandemir
Copy link
Owner

@eboye did you forget to push your changes, or will you push them later? 🙂

@eboye
Copy link
Contributor Author

eboye commented Feb 27, 2022

Sorry, here they are.

The only thing I want to mention is that this one test fails after my tinkering:

TypeError : PHPUnit\Framework\Assert::assertContains(): Argument #2 ($haystack) must be of type iterable, string given, called in ~/GitHub/microdata-parser/tests/MicrodataParserTest.php on line 82
~/GitHub/microdata-parser/tests/MicrodataParserTest.php:82

@yusufkandemir yusufkandemir merged commit 59fcb3f into yusufkandemir:master Apr 1, 2022
@yusufkandemir
Copy link
Owner

@eboye thanks for the contributions! I made more changes, migrated to Pest from PHPUnit, migrated to GitHub actions from all other CI tools, and improved the code quality, then released https://github.com/yusufkandemir/microdata-parser/releases/tag/v1.0.0, enjoy!

@eboye
Copy link
Contributor Author

eboye commented Apr 3, 2022

Great!

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.

PHP 8 compatibility
2 participants