Skip to content

Conversation

@calebdw
Copy link
Contributor

@calebdw calebdw commented Jul 27, 2025

Hello!

Closes rectorphp/rector#9289

Motivation

Converting ArrayDimFetch (for objects that implement ArrayAccess) can be very dangerous if the isset(), unset, and assignment operations are not accounted for. If the object is used in any of the following scenarios:

$object['key'] = 42;
isset($object['key']);
unset($object['key']);

this rule would update them to the following and the application will be in a broken state:

$object->get('key') = 42;
isset($object->get('key'));
unset($object->get('key'));

Implementation

This rule properly handles all of the above situations (including when isset and unset have multiple arguments). The ArrayDimFetchToMethodCall class has been updated to accept methods for the other operations:

class ArrayDimFetchToMethodCall
{
    public function __construct(
        private readonly ObjectType $objectType,
        private readonly string $method,
        // Optional methods for set, exists, and unset operations
        // if null, then these operations will not be transformed
        private readonly ?string $setMethod = null,
        private readonly ?string $existsMethod = null,
        private readonly ?string $unsetMethod = null,
    ) {
    }
}

Note

I would like to rename the existing $method parameter to $getMethod, let me know if this is allowable and not considered a BC break

The other methods are nullable by default. If null, then that particular operation is skipped.

CC: @peterfox

Thanks!

@calebdw calebdw force-pushed the calebdw/push-zvpyxrvzmsol branch from d2db052 to b749d9a Compare July 27, 2025 02:33
@calebdw calebdw force-pushed the calebdw/push-zvpyxrvzmsol branch from b749d9a to b898cff Compare July 27, 2025 04:06
@samsonasik
Copy link
Member

/cc @peterfox please review this when you have a chance, thank you.

@calebdw
Copy link
Contributor Author

calebdw commented Jul 30, 2025

@samsonasik, @peterfox tests are passing, any chance we could try to get this merged this week?

Also, what do you think about this note?

image

Thanks!

@TomasVotruba
Copy link
Member

Looks good to me as it is, thanks 👍

@TomasVotruba TomasVotruba merged commit 4dfb209 into rectorphp:main Jul 30, 2025
47 checks passed
@calebdw calebdw deleted the calebdw/push-zvpyxrvzmsol branch July 30, 2025 12:47
@calebdw
Copy link
Contributor Author

calebdw commented Jul 30, 2025

Thanks!

@github-actions
Copy link
Contributor

This pull request has been automatically locked because it has been closed for 150 days. Please open a new PR if you want to continue the work.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2025
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.

Incorrect behavior of ArrayDimFetchToMethodCallRector

3 participants