php-ds / polyfill Goto Github PK

Am I right that that the fallback value for $default in Map::get() can never be used?
https://github.com/php-ds/polyfill/blob/master/src/Map.php#L320

If the method is called without its second argument (to get the fallback value), the call will fail here (as confirmed by the testGetKeyNotFound test).

In that case, I'd say it should be removed from the signature.

(Or maybe there's a PHP trick that I'm not thinking about?)

Hi!

I just released a library to handle Tree data structures using Ds Deque class ho handle dynamic increase/decrease of childrens.

What I found interesting is the following:

• before installing the extension on TravisCI unit-tests didn't work
• after installing the extension everything came back to normal.

During the development I used the extension, perhaps because I am a bit nostalgic of C++ routines and also because I believe that it is more efficient as behind the hood uses dynamic lists instead of php arrays...

However, I cannot find the problem at the moment, but I start by creating this issue.

Later I'll try myself to dig inside your project to understand why I got an infinite recursion when using Php version instead of C++ extension.

For the ones interesting in the library I was talking about, have a look here:

https://github.com/calabrothers/php-ds-tree

With my best regards

Hello, how would you feel about tagging a new release? The latest one is over three years old and I've noticed a bunch of bugfixes have been merged into master since then.

Thanks for your consideration :)

As its currently not possible to install the C extension in some cases, it would
be helpful to be able to install the polyfill only.

If thats already possible, it would be helpful to put instructions on how to do
that.

If anyone would like to implement an object-key supported insertion-ordered constant lookup hash table in PHP let me know. 🐌

Hi @rtheunissen.

I've forked and cloned both the ployfill and tests repositories and looking to start the implementation.

If we are to support php5.6 then we need to remove scalar type hinting and return type declarations. Doing this has highlighted to me a problem with having the tests in a separate repo.

For me to work on the polyfill and also run the tests I've done the following:

1. Cloned the polyfill
2. Autoload the tests using composer autoload-dev
3. Created a new phpunit.xml file in the polyfill that uses composer/autoload.php as the bootstrap.

At this stage I can run tests (albeit failing).

But to work on the tests I need to use my fork of the tests repo. After running composer, I removed the contents of ./vendor/php-ds/tests and symlinked my fork of the test repo to that location.
Now I can update tests as I modify the polyfill but this process is terrible.

When it then comes to creating a pull request I'd need to create requests for both the tests and polyfill. Then the CI will need a way to combine the repos together correctly to confirm the code is sound and the merge can be accepted.

I can't see how that this can work in a way that people will be able to contribute without having the tests committed directly in the polyfill.

What are your thoughts?

I just came across an interesting bug I thought you might like.

I have a Set that is being created using an array like this pseudo code

foreach ($users as &$userId) {
    $userId = UserId::fromString($userId->user_uuid);
}

$result = new Set($users);

UserId implements Hashable.

When I come to add another UserId to the Set, even if the hash functions return the same value, and the equals functions return true, the new UserId is added to the Set.

Set after loop and before add:

class Ds\Set#41 (3) {
  public ${0} =>
  class ThoughtRiver\Review\Domain\Model\User\UserId#40 (1) {
    private $identity =>
    string(36) "4f220a85-1b2f-40f6-91c8-5879a8331c9f"
  }
  public ${1} =>
  class ThoughtRiver\Review\Domain\Model\User\UserId#42 (1) {
    private $identity =>
    string(36) "5e006610-5ed4-4393-a6e7-57857ac5deec"
  }
  public ${2} =>
  class ThoughtRiver\Review\Domain\Model\User\UserId#43 (1) {
    private $identity =>
    string(36) "b6f23619-dfca-423f-b598-3878a30abbc1"
  }
}

Set after loop and after add

class Ds\Set#41 (4) {
  public ${0} =>
  class ThoughtRiver\Review\Domain\Model\User\UserId#40 (1) {
    private $identity =>
    string(36) "4f220a85-1b2f-40f6-91c8-5879a8331c9f"
  }
  public ${1} =>
  class ThoughtRiver\Review\Domain\Model\User\UserId#42 (1) {
    private $identity =>
    string(36) "5e006610-5ed4-4393-a6e7-57857ac5deec"
  }
  public ${2} =>
  class ThoughtRiver\Review\Domain\Model\User\UserId#43 (1) {
    private $identity =>
    string(36) "b6f23619-dfca-423f-b598-3878a30abbc1"
  }
  public ${3} =>
  class ThoughtRiver\Review\Domain\Model\User\UserId#32 (1) {
    private $identity =>
    string(36) "5e006610-5ed4-4393-a6e7-57857ac5deec"
  }
}

Notice indexes 1 and 3

If I change the loop to the following:

$result = new Set;

foreach ($users as $userId) {
    $result->add(UserId::fromString($userId->user_uuid));
}

Then I cannot add a new UserId to the Set when the Set already contains an object that equals considers to be true.

How weird?

Map does not have this method but php-ds says that it has this method

Hi,

I want use this polyfill in library which needs support php 5.6+. Please how can I do that?

Thank you for answer.

Pavel

The Set::map() method is not implemented in polyfill although it was added in version 1.2.7 of php-ds/ext-ds project.

This error was caused by another error in the php-ds/tests project: php-ds/tests#19

Hello,

If the polyfill is installed when the extension is available, which of both is used? Is the polyfill code prepared to give preference to the extension classes?

DocBlock saids it returns a Map but it does not.

It is worth mentioning that for those who are sure that the DS extension is loaded but still want polyfill for better IDE autocompletion, composer require --dev is a better option.

Hi @rtheunissen,

Running the tests against the polyfill I see the following error:

1) Ds\Tests\DequeTest::testEcho
Object of class Ds\Deque could not be converted to string

/Users/dantudor/Sites/php-ds/tests/tests/CollectionTest.php:43
/Users/dantudor/Sites/php-ds/tests/tests/Sequence/_echo.php:8

I'm not sure of the purpose of this test as the Collection interface doesn't describe the need for __toString();

Is there a recommendation for this? At the moment I have simply *, is that sufficient?

Hi there!

I'm implementing a middleware pipeline third-party library using this extension with the polyfill. The implementation exhausts the Queue class of this library recursively by cloning itself. You can take a look at the implementation here.

When I run the tests with the extension, everything works perfectly well and as expected. However, when I run the tests with the polyfill, everything breaks.

Looking at the polyfill code, I realized that there was not a Ds\Queue::__clone method implemented, so when I called clone on the queue, the internal Ds\Deque containing the array was not being cloned.

I implemented the clone method in my vendor folder just for testing purposes, and this fixed it.

I can submit a PR with the fix if you are busy to fix it yourself. Just let me know.

Thanks for this awesome extension!

Hello,

I know, it's a little error, but current version of composer.json is not valid, because it does not satify the JSON schema validation !

Package description is missing.

composer validate command produced such results

./composer.json is valid for simple usage with Composer but has
strict errors that make it unable to be published as a package
See https://getcomposer.org/doc/04-schema.md for details on the schema
# Publish errors
- description : The property description is required

PHP Deprecated:  Return type of Ds\Vector::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /Users/john/Projects/jorpo-co/concerto/concerto-api/vendor/php-ds/php-ds/src/Traits/GenericSequence.php on line 355

This is with the polyfill on PHP 8.1, I will check now with the extension.

ArrayAccess doesn't support $array[][] syntax, resulting in an Indirect modification of overloaded element of X has no effect warning.

This may be a dealbreaker.

Subj: https://packagist.org/packages/php-ds/

Can you add this into packagist?

These interfaces and classes implement ArrayAccess in the extension:

$classes = array_merge(get_declared_interfaces(), get_declared_classes());

foreach ($classes as $class) {
    if (substr($class, 0, 3) === 'Ds\\') {
        if ((new ReflectionClass($class))->implementsInterface(ArrayAccess::class)) {
            echo "$class\n";
        }
    }
}

Ds\Sequence
Ds\Vector
Ds\Deque
Ds\Stack
Ds\Map

However, I can see that the following classes/interfaces implement ArrayAccess in the polyfill:

Ds\Queue
Ds\Stack
Ds\Sequence
Ds\Set
Ds\Map

In short, the following classes implement ArrayAccess in the polyfill but not in the extension (at least, according to reflection):

• Queue
• Set

Is this an issue with the polyfill or the extension?

The code within the Sequence Trait constructor can expect the $values argument to be an int and in that case it's used to set the capacity of the object.

/**
 * Creates a new sequence using the values of either an array or iterable
 * object. The keys of either will not be preserved.
 *
 * @param array|\Traversable $values
 */
function __construct($values = null) {
    if ($values) {
        if (is_integer($values)) {
            $this->allocate($values);
        } else {
            $this->pushAll($values);
        }
    }
}

This isn't what the DocBlock suggests. We should update the DocBlock to include allowing array|\Travesable|int|null as allowed argument types?

HI there!

When extension is enabled:

serialize(new Ds\Set()); // result - 'C:6:"Ds\Set":0:{}'

when extension is disabled

serialize(new Ds\Set()); // result -  'O:6:"Ds\Set":1:{s:13:" Ds\Set table";O:6:"Ds\Map":2:{s:13:" Ds\Mappairs";a:0:{}s:16:" Ds\Map capacity";i:8;}}'

And if serialize data when extension is enabled and unserialize when extension is disabled(or opposite) i have warning Warning: Class Ds\Set has no unserializer

Would it be at all possible to have Hashable split out into a separate repository that doesn't require PHP 7+ ?

I have an old codebase that I want to migrate slowly over to newer concepts, and would like to implement Hashable already, so when I can upgrade the PHP environment, I am already half way there.

Default packages I maintain for the project, like ValueObjects, are already separated out for re-use and were implemented in a newer project running on PHP 7, but out old code can't use these classes as they use Hashable.

This will reduce some confusion when seeing two classes both named Collection in such a small codebase.

I've encountered an issue that eg. Set::getIterator() is present in polyfill but not in ext-ds. What is intended?

Further to #97, there are still more deprecation warnings for PHP 8.1.

User Deprecated: Method "IteratorAggregate::getIterator()" might add "\Traversable" as a native return type declaration in the future. Do the same in implementation "Ds\Vector" now to avoid errors or add an explicit @return annotation to suppress this message.
User Deprecated: Method "ArrayAccess::offsetSet()" might add "void" as a native return type declaration in the future. Do the same in implementation "Ds\Vector" now to avoid errors or add an explicit @return annotation to suppress this message.
User Deprecated: Method "ArrayAccess::offsetGet()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "Ds\Vector" now to avoid errors or add an explicit @return annotation to suppress this message.
User Deprecated: Method "ArrayAccess::offsetUnset()" might add "void" as a native return type declaration in the future. Do the same in implementation "Ds\Vector" now to avoid errors or add an explicit @return annotation to suppress this message.
User Deprecated: Method "ArrayAccess::offsetExists()" might add "bool" as a native return type declaration in the future. Do the same in implementation "Ds\Vector" now to avoid errors or add an explicit @return annotation to suppress this message.
User Deprecated: Method "IteratorAggregate::getIterator()" might add "\Traversable" as a native return type declaration in the future. Do the same in implementation "Ds\PriorityQueue" now to avoid errors or add an explicit @return annotation to suppress this message.

The polyfill implementation of \Ds\Map->map() loses the keys of the input map, unlike the Extension. I know the polyfill is somewhat-deprecated, and we're switching to the extension, but the incorrectness caught us by surprise.

eg.

$map = new \Ds\Map(["speed" => 5, "greatness" => 6]);
$mapped = $map->map(function ($key, $value) {
  return $value * 2;
});
echo $mapped->toArray(); // expected: ["speed" => 10, "greatness" => 12], actual: [10, 12]

This is kind of a question as well as a feature request.

I would like to see the methods key and value added to the Pair class to make it easy to retrieve the two from the object.

Do you think this is feasible? Should I log this on the main repo instead?