DatePeriod::getIterator()->valid() advances DatePeriod iterator #7979

Activity

1. dg

   added

   Bug

   Status: Needs Triage

   on Jan 21, 2022
2. drupol commented on Jan 21, 2022

   

   drupol

   on Jan 21, 2022

   Contributor

   More actions

   Same issue in PHP 8.1.1
3. cmb69 commented on Jan 21, 2022

   

   cmb69

   on Jan 21, 2022

   Member

   More actions

   Yeah, that looks wrong: https://3v4l.org/sNs51. This appears to be related to https://bugs.php.net/50981, but contrary to that issue, this really looks like an implementation bug. Anyhow, use ::isValid() as workaround.
4. cmb69

   added

   Extension: spl

   Status: Verified

   and removed

   Status: Needs Triage

   on Jan 21, 2022
5. aragon999 commented on Jan 22, 2022

   

   aragon999

   on Jan 22, 2022

   Last edited by aragon999

   Contributor

   More actions

   For a short weekend distraction I looked a bit around in the source code which seems to be responsible for this bug.

   It seems that this bug only occurs with InternalIterators. For example the following code (from a comment in the PHP docs) seems to not be affected by this issue:
   https://3v4l.org/P6icu

   Since the CachingIterator checks that the element of the internal iterator (which is one step ahead in the actual iteration) is valid, I then looked into the code which is responsible for the validation in the case of an InternalIterator:
   

   php-src/Zend/zend_interfaces.c

   Lines 587 to 600 in 024d5f4

   	ZEND_METHOD(InternalIterator, valid) {
   	ZEND_PARSE_PARAMETERS_NONE();
   	zend_internal_iterator *intern = zend_internal_iterator_fetch(ZEND_THIS);
   	if (!intern) {
   	RETURN_THROWS();
   	}
   	if (zend_internal_iterator_ensure_rewound(intern) == FAILURE) {
   	RETURN_THROWS();
   	}
   	RETURN_BOOL(intern->iter->funcs->valid(intern->iter) == SUCCESS);
   	}

   In particular the zend_internal_iterator_ensure_rewound function
   

   php-src/Zend/zend_interfaces.c

   Lines 514 to 526 in 024d5f4

   	static zend_result zend_internal_iterator_ensure_rewound(zend_internal_iterator *intern) {
   	if (!intern->rewind_called) {
   	zend_object_iterator *iter = intern->iter;
   	intern->rewind_called = 1;
   	if (iter->funcs->rewind) {
   	iter->funcs->rewind(iter);
   	if (UNEXPECTED(EG(exception))) {
   	return FAILURE;
   	}
   	}
   	}
   	return SUCCESS;
   	}

   
   seems to be responsible for the faulty behavior described here.

   Unfortunately I have no idea how to fix this problem, since I lack understanding how it should be addressed.
6. TysonAndre commented on Feb 7, 2022

   

   TysonAndre

   on Feb 7, 2022

   Contributor

   More actions

   <?php
   
   $start = \DateTime::createFromFormat('Y-m-d', '2021-01-01');
   $end = \DateTime::createFromFormat('Y-m-d', '2021-01-10');
   $period = new \DatePeriod($start, \DateInterval::createFromDateString('1 day'), $end);
   $iterator = $period->getIterator();
   
   foreach ($iterator as $item) {
   	echo $item->format('c'), "\n";
   	$iterator->valid(); // advances iterator
   	$iterator->valid(); // advances iterator
   }

   This looks like an issue with DatePeriod::getIterator, not CachingIterator. https://3v4l.org/Er6ej
7. TysonAndre

   added

   Extension: date

   and removed

   Extension: spl

   on Feb 7, 2022
8. TysonAndre

   changed the title [-]CachingIterator::hasNext() advances DatePeriod iterator[/-] [+]DatePeriod::valid() advances DatePeriod iterator[/+] on Feb 7, 2022
9. TysonAndre

   changed the title [-]DatePeriod::valid() advances DatePeriod iterator[/-] [+]DatePeriod::getIterator()->valid() advances DatePeriod iterator[/+] on Feb 7, 2022
10. TysonAndre commented on Feb 7, 2022

    

    TysonAndre

    on Feb 7, 2022

    Last edited by TysonAndre

    Contributor

    More actions

    Specifically, it's caused by /* apply modification if it's not the first iteration */ in ext/date/php_date.c

    1. It shouldn't be modifying state (or not modifying more than once?) in valid()
    2. It should be modifying state in rewind, construction, and move_forward, I believe? Not familiar with this extension's implementation details.

    // ext/date/php_date.c
    /* iterator handler table */
    static const zend_object_iterator_funcs date_period_it_funcs = {
    	date_period_it_dtor,
    	date_period_it_has_more,
    // ...
    
    
    /* {{{ date_period_it_has_more */
    static int date_period_it_has_more(zend_object_iterator *iter)
    {
    // ext/date/php_date.c
    	date_period_it *iterator = (date_period_it *)iter;
    	php_period_obj *object   = Z_PHPPERIOD_P(&iterator->intern.data);
    	timelib_time   *it_time = object->current;
    
    	/* apply modification if it's not the first iteration */
    	if (!object->include_start_date || iterator->current_index > 0) {
    		it_time->have_relative = 1;
    		it_time->relative = *object->interval;
    		it_time->sse_uptodate = 0;
    		timelib_update_ts(it_time, NULL);
    		timelib_update_from_sse(it_time);
    	}
    
    	if (object->end) {
    		return object->current->sse < object->end->sse ? SUCCESS : FAILURE;
    	} else {
    		return (iterator->current_index < object->recurrences) ? SUCCESS : FAILURE;
    	}
    }
    /* }}} */
11. Nacoma

    added a commit that references this issue on Mar 6, 2022

    phpGH-7979: iterator advances when checking if valid

    1edfafa
12. cmb69

    linked a pull request that will close this issueFix GH-7979: DatePeriod iterator advances when checking if valid #8175on Mar 30, 2022
13. derickr

    self-assigned this

    on Apr 1, 2022
14. derickr

    added a commit that references this issue on Apr 14, 2022

    GH-7979: iterator advances when checking if valid

    287c8a8
15. derickr commented on Apr 14, 2022

    

    derickr

    on Apr 14, 2022

    Member

    More actions

    This has been fixed in Git, and will be part of next month's releases.

    👍React with 👍1Reacted by Michal Landsman
16. derickr

    closed this as completedon Apr 14, 2022