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

Add PHP8.1 Compatibility #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jakejackson1
Copy link

@jakejackson1 jakejackson1 commented Oct 14, 2022

This PR:

  • Fixes a number of type-related errors on PHP 8.1
  • Update PHPUnit Test Suite to pass from PHP7.1 to 8.1
  • Add GitHub Actions to run PHPUnit for all PRs
  • Removes composer.lock file to ensure the correct PHPUnit is installed in GitHub Actions. Since this is a library this is generally the accepted practice.
  • Removed the Faker dependency (it didn't appear to be used in the codebase and has been sunset).

Note: The GitHub Actions won't get run until merged into the master branch, but you can see they all pass in my fork.

I'll create another PR that includes a linter, fixes up any lint errors, and will do automatic linting on the library on commits using GitHub Actions too.

Add GitHub Actions and update PHPUnit Test Suite to pass from PHP7.1 to 8.1.
@jakejackson1
Copy link
Author

jakejackson1 commented Oct 14, 2022

Once this is merged I'll create this PR jakejackson1#2 upsteam 👍

@jakejackson1
Copy link
Author

Hey @arthurkushman. Any feedback on this PR?

@kevinjavitz
Copy link

Seems a good PR, why isn't this being merged? I'm using it on our module and it fixes the PHP 8.1 issues.

@jakejackson1
Copy link
Author

@salesigniter could be @arthurkushman is no longer available, hasn't had the time to review and merge, or is no longer interested in maintaining the package. If nothing is heard soon I'll look at creating another fork of the library and publishing on Packagist.

@kevinjavitz
Copy link

Ok, thanks to you both for your hard work on this 🙂

@luigimannoni
Copy link

Hi @jakejackson1
has this been published on packagist? There's another package published with 8.1 support ( Satisfactory-Clips-Archive/querypath ) but it has some extras I am not comfortable with and yours seem to be a valid solution.

@jakejackson1
Copy link
Author

jakejackson1 commented Dec 1, 2022

Hi @jakejackson1 has this been published on packagist? There's another package published with 8.1 support ( Satisfactory-Clips-Archive/querypath ) but it has some extras I am not comfortable with and yours seem to be a valid solution.

@luigimannoni no. I was hoping @arthurkushman would merge this PR so a fork wasn't necessary, but it's becoming more and more likely that is what I will need to do.

@luigimannoni
Copy link

Thanks, that would be helpful. I would do it myself through the packagist website but I'd rather leave you to take credit for it.

If you decide to publish it, can you add a replace directive on the composer.json? It'll help project dependencies to fetch your version of the package instead the non-working one.

Thanks again

@jakejackson1
Copy link
Author

jakejackson1 commented Dec 7, 2022

@luigimannoni I have finished forking QueryPath and publishing to Packagist https://packagist.org/packages/gravitypdf/querypath

The fork includes the PHP8.1 work in this PR, code formatting from jakejackson1#2, and a bunch of changes to ensure our team and the development community can easily report issues and contribute to the library. See https://github.com/GravityPDF/querypath/releases/tag/3.2.0 for a summary.

Note: we did a manual fork of QueryPath so that GitHub doesn't automatically create pull requests upstream every time a PR is opened (which can't be turned off as far as I can tell). This will make it easily to maintain long term. We ensured the manual fork included the branches and tags, as well as relevant issues from the original repo and the fork.

@luigimannoni
Copy link

Thanks @jakejackson1
I've updated the AMP library pull request here Lullabot/amp-library#310 and notified people at drupal/feeds_ex and drupal/amp

@luigimannoni
Copy link

luigimannoni commented Dec 8, 2022

Getting the following when fetching the iterator

Deprecated function: Return type of QueryPath\QueryPathIterator::current() should either be compatible with IteratorIterator::current(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in include() (line 571 of /app/vendor/composer/ClassLoader.php)
#0 /app/web/core/includes/bootstrap.inc(347): _drupal_error_handler_real(8192, 'Return type of ...', '/app/vendor/gra...', 29)
#1 /app/vendor/composer/ClassLoader.php(571): _drupal_error_handler(8192, 'Return type of ...', '/app/vendor/gra...', 29)
#2 /app/vendor/composer/ClassLoader.php(571): include('/app/web/core/i...')
#3 /app/vendor/composer/ClassLoader.php(428): Composer\Autoload\includeFile('/app/vendor/com...')
#4 /app/vendor/gravitypdf/querypath/src/DOMQuery.php(1564): Composer\Autoload\ClassLoader->loadClass('QueryPath\\Query...')
#5 /app/vendor/deimosindustries/lullabot-amp/src/Pass/ImgTagTransformPass.php(66): QueryPath\DOMQuery->getIterator()

from what I can understand:

public function getIterator(): \Traversable

changes its typehint to

public function getIterator(): mixed

or perhaps a pseudo iterable?

This is triggering from a simple foreach:

$all_images = $this->q->top()->find('img:not(noscript img)');
if ( ($all_images instanceof \Traversable) ) {
    /** @var DOMQuery $element */
    foreach ($all_images as $element) {
            [...]
    }
}

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.

4 participants