Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Initial Classes #1

Merged
merged 5 commits into from
Mar 21, 2018
Merged

Initial Classes #1

merged 5 commits into from
Mar 21, 2018

Conversation

XedinUnknown
Copy link
Member

Adds the initial classes.

@XedinUnknown XedinUnknown added this to the 0.1 milestone Mar 19, 2018
@XedinUnknown XedinUnknown self-assigned this Mar 19, 2018
@XedinUnknown XedinUnknown requested a review from mecha March 19, 2018 15:42
@mecha
Copy link
Member

mecha commented Mar 19, 2018

I ran the tests on PHP 5.4.0 and got an error.

PHPUnit_Framework_Error_Notice: ArrayIterator::key(): Array was modified outside object and internal position is no longer valid.

Did some research and it looks like it's this bug, which was fixed in PHP 5.4.6:
https://bugs.php.net/bug.php?id=61527

@mecha
Copy link
Member

mecha commented Mar 20, 2018

After some research, discovered the following:

The error is actually a Dhii\Iterator\Exception\IteratorException with the mesage:

An error occurred while iterating

It's caused by an exception of type PHPUnit_Framework_Error_Notice, which I'm guessing uses an error handler to throw instead of triggering notices. The message is the one I reported earlier:

ArrayIterator::key(): Array was modified outside object and internal position is no longer valid.

The stack trace for this inner exception reveals that the error is caused when calling ArrayIterator::key() at AbstractBaseMap#214.

@mecha
Copy link
Member

mecha commented Mar 20, 2018

Identified the problem. It lies in AbstractBaseMap::_loop()

When PHP iterates an iterator, it follows the following algo:

$iter->rewind();
while ($iter->valid()) {
	$val = $iter->current();
	$key = $iter->key();
	// yield $val and $key
	$iter->next();
}

Was also surprised that it retrieves the value before the key - for some reason I'd thought it would be the other way around, but I digress.

However, note that when PHP invokes rewind(), the first $key and $val are expected to be available without the need to do an initial next().

The AbstractBaseMap::_loop() is called on next() and does the following:

protected function _loop()
{
	$iterator = $this->_getIterator();

	$iterator->next();
	$iteration = $this->_createIteration($iterator->key(), $iterator->current());

	return $iteration;
}

Effectively, the iteration object is created on next(), but when next() is not guaranteed to have a next element to advance to. This is especially true for PHP iterators.

Prior to PHP 5.4.6, reaching the end of a PHP iterator meant a notice would be triggered. After PHP 5.4.6, this was changed to yield a null key and null value, similar to how some Dhii iterators do it. PHP then understands that this means the end has been reached. The notice was also removed.

The solution is to amend the above method to, after invoking $iterator->next(), also check if it the $iterator is ->valid(). If not, an iteration with null key and null value should be returned.

I will leave a formal review where it is applicable.

tl;dr

Prior to 5.4.6, $iterator->key() and $iterator->current() triggered a notice when the end was reached.

As of 5.4.6, $iterator->key() and $iterator->current() both return null when the end was reached.

$iterator = $this->_getIterator();

$iterator->next();
$iteration = $this->_createIteration($iterator->key(), $iterator->current());
Copy link
Member

Choose a reason for hiding this comment

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

Check if the $iterator is still valid after advancing. If not, the key and value of the created iteration should be null.

This change would make this package compatible with PHP 5.4.0, otherwise it is only compatible with PHP 5.4.6 or later.

... iterator reaches the end.
@XedinUnknown XedinUnknown merged commit 2dff235 into develop Mar 21, 2018
@XedinUnknown XedinUnknown deleted the task/initial-classes branch March 21, 2018 16:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants