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

Fix service registration delay causing next service to be skipped #6529

Merged
merged 3 commits into from
Aug 13, 2021

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Aug 11, 2021

Summary

Fixes the regression @westonruter reported in #6459 (comment).

When a service's registration is delayed, the service is moved to the end of the array list of services to be registered. When the service is moved to the end, the array's internal pointer moves to the next service in the array, pointing to the next service to be registered. By calling next($services) the array's internal pointer is moved once again to the next service, skipping the to-be service and preventing it from being registered.

See #6484.
Fixes #6459.

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@pierlon pierlon added the Bug Something isn't working label Aug 11, 2021
@pierlon pierlon added this to the v2.1.4 milestone Aug 11, 2021
@pierlon pierlon requested a review from westonruter August 11, 2021 16:02
@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2021

Plugin builds for 0fc0374 are ready 🛎️!

@westonruter
Copy link
Member

The symptom is fixed:

image

But tests are failing due to an apparent infinite loop/recursion. Running tests locally I'm getting the same issue as GHA:

.....................S....................................... 2013 / 2439 ( 82%)
................................S.........................Killed

@pierlon
Copy link
Contributor Author

pierlon commented Aug 12, 2021

But tests are failing due to an apparent infinite loop/recursion.

Fixed in 0fc0374.

@westonruter westonruter requested a review from schlessera August 13, 2021 18:04
@westonruter westonruter merged commit 11e6601 into develop Aug 13, 2021
@westonruter westonruter deleted the fix/skipping-next-service branch August 13, 2021 19:13
@westonruter
Copy link
Member

@schlessera If you could double check the changes here when you get a chance. In the meantime I'm merging because it fixes the issue.

@schlessera
Copy link
Collaborator

That definitely looks off, and I think it only happens to work because we don't have a service yet that has more than one delayed requirement.

If you look at the structure, you can see that the next($services) is located within the foreach ( $missing_requirements as $missing_requirement ) loop, meaning that if you check requirements for a service that has 3 delayed dependencies, it will forward the index into the $services array by 3 entries... that can't be right.

@schlessera
Copy link
Collaborator

In case the logic is too complex and finicky with the array index, we can work with a copy of the array that we pop/shift elements out of. That will probably make for cleaner code, even if at a slight performance overhead.

@schlessera
Copy link
Collaborator

Ah, nevermind. Due to the return false in the loop, this will never be executed more than once.

@schlessera
Copy link
Collaborator

The fix LGTM, but I noticed another problem and opened #6548 for that.

@westonruter westonruter self-assigned this Aug 30, 2021
@westonruter
Copy link
Member

QA Passed

The ValidationCounts service is initializing as expected:

image

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin fails to activate using TGMPA
3 participants