Skip to content

Conversation

@Fenikkusu
Copy link
Contributor

@Fenikkusu Fenikkusu commented May 6, 2025

Hello!

  • Type: bug fix
  • Link to issue: N/A

In raising this pull request, I confirm the following:

  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • I updated the CHANGELOG

Small description of change:

Taking a Stab at fixing the issue where calling a closure with a use statement within a loop always uses the last value:

IE: (Psudo Code)

class Something extends Injectable {
    function doSomething(string! name, int value) {
        this->getDi() ->set(name, function() use (value) {
            return value;
        });
    }
}

var something = new Something();
something->doSomething("a", 1);
something->doSomething("b", 2);

// Current
di->get("a"); // Returns 2
di->get("b"); // Returns 2

// Should Be
di->get("a"); // Returns 1
di->get("b"); // Returns 2

I'm posting this here as I have no clue what I'm doing, and someone smarter than I may be able to make quick changes to this to finish it up. My latest change was to Method and stubs stopped building as a result and I don't have the time to keep digging ATM. Otherwise, I will keep working on it as time permits.

@Fenikkusu Fenikkusu force-pushed the hotfix/closure-use-loop branch from 574f3c8 to 3151d7b Compare June 3, 2025 18:06
@Fenikkusu Fenikkusu marked this pull request as ready for review June 3, 2025 18:07
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 3, 2025

@Fenikkusu
Copy link
Contributor Author

Okay, I believe I have gotten this completed correctly now. I've split the original changes into multiple PRs specific to the needs of each ( #2456 & #2455 ), though one is no longer needed. Unfortunately, the Windows Errors are outside the scope of my focus at the moment, so someone else will need to handle those issues.

@Fenikkusu
Copy link
Contributor Author

Documenting: Attempting to compile Phalcon using this version appears to fail on my end. Once I confirm the issue, I will post an update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant