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: Remove obsolete resource typing #44658

Merged
merged 8 commits into from
Apr 16, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Apr 4, 2024

Summary

In PHP>=8.1, LDAP and FTP resources are always typed objects

TODO

Checklist

@come-nc come-nc added the 2. developing Work in progress label Apr 4, 2024
@come-nc come-nc self-assigned this Apr 4, 2024
@come-nc come-nc force-pushed the fix/migrate-away-from-resource-type branch from 5ba403a to cd68ed3 Compare April 8, 2024 12:41
In PHP>=8.1, LDAP and FTP resources are always typed objects

Signed-off-by: Côme Chilliet <[email protected]>
Directly copied stubs for php ext folder from PHP-8.1 branch

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc force-pushed the fix/migrate-away-from-resource-type branch from f082338 to 8177fc8 Compare April 9, 2024 08:01
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 9, 2024
@come-nc come-nc added this to the Nextcloud 30 milestone Apr 9, 2024
@come-nc come-nc requested review from blizzz, a team, icewind1991, yemkareems, sorbaugh, nickvergessen and kesselb and removed request for a team April 9, 2024 12:24
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Nice 🙏

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

One thing to doublecheck

} while (($state === self::LRESULT_PROCESSED_SKIP
|| $this->ldap->isResource($entry))
&& ($dnReadLimit === 0 || $dnReadCount < $dnReadLimit));
} while ($dnReadLimit === 0 || $dnReadCount < $dnReadLimit);
Copy link
Member

Choose a reason for hiding this comment

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

You are not taking $state into account anymore. Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The call to $this->ldap->isResource($entry) cannot return false, by typing, so the $state was not taken into account before either. If this is/was a bug, we can change the condition.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Right. I was a bit puzzled also because the local $state var was not removed (NaaS). All good.

build/stubs/ldap.php Show resolved Hide resolved
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🐘

@come-nc come-nc merged commit 3ad4bbb into master Apr 16, 2024
159 checks passed
@come-nc come-nc deleted the fix/migrate-away-from-resource-type branch April 16, 2024 15:13
@blizzz blizzz mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants