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

Replace jeremeamia/superclosure with opis/closure #37238

Merged
merged 1 commit into from
Apr 13, 2020
Merged

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Apr 11, 2020

Description

jeremeamia/superclosure library is no longer maintained and is only officially tested up to PHP 7.2.
https://github.com/jeremeamia/super_closure/blob/master/README.md

Replace it with the recommended https://github.com/opis/closure library.

composer update
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 1 install, 0 updates, 3 removals
  - Removing symfony/polyfill-util (v1.15.0)
  - Removing symfony/polyfill-php56 (v1.15.0)
  - Removing jeremeamia/superclosure (2.4.0)
  - Installing opis/closure (3.5.1): Loading from cache

This happens to also get rid of some symfony/polyfill dependencies.
And means we are using a dependency that is officially tested on PHP 7.3 and 7.4

How Has This Been Tested?

CI and local run of unit tests:

make test-php-unit TEST_PHP_SUITE=tests/lib/Command/AsyncBusTest.php

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@phil-davis phil-davis added 3 - To Review dependencies php Pull requests that update Php code labels Apr 11, 2020
@phil-davis phil-davis self-assigned this Apr 11, 2020
@codecov
Copy link

codecov bot commented Apr 11, 2020

Codecov Report

Merging #37238 into master will increase coverage by 0.00%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37238   +/-   ##
=========================================
  Coverage     64.92%   64.92%           
- Complexity    19148    19149    +1     
=========================================
  Files          1267     1267           
  Lines         74895    74896    +1     
  Branches       1331     1331           
=========================================
+ Hits          48626    48627    +1     
  Misses        25877    25877           
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 66.12% <87.50%> (+<0.01%) 19149.00 <0.00> (+1.00)
Impacted Files Coverage Δ Complexity Δ
lib/private/Command/ClosureJob.php 87.50% <85.71%> (+4.16%) 3.00 <0.00> (+1.00)
lib/private/Command/AsyncBus.php 91.66% <100.00%> (-0.23%) 19.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73fa496...586693a. Read the comment docs.


class ClosureJob extends QueuedJob {
protected function run($serializedCallable) {
$serializer = new Serializer();
$callable = $serializer->unserialize($serializedCallable);
$serializedClosure = \unserialize($serializedCallable);
Copy link
Member

Choose a reason for hiding this comment

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

compare result with false to avoid getClosure() on false below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used method_exists because it is also nice to know that the unserialized "thing" is an object that has the required method. I checked that if $serializedClosure is not even an object (e.g. is false or "abc" or...) then method_exists happily returns false.

@phil-davis phil-davis merged commit 6ca2fa5 into master Apr 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the opis-closure branch April 13, 2020 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - To release dependencies php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants