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

[4.4] Converting from File::exists() to is_file() #40113

Merged
merged 4 commits into from
Mar 15, 2023

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Mar 14, 2023

Summary of Changes

In the process of deprecating the CMS Filesystem package, this PR replaces all cases of File::exists() with is_file(). File::exists() is a wrapper for is_file(Path::clean($path)), so by removing it, we are removing the Path::clean($path). However, as far as I can see, we either have a safely secure path anyway or it is already wrapped with Path::clean(). Where it isn't wrapped, it actually is hardly usefull, because we do a File::exists($path) and shortly after use the original value of $path to include that file. Long story short, I think this is a safe change.
I still would like the @joomla/security-leads to have a look at this.

Testing Instructions

After applying this, nothing should have changed. Thus, a codereview would probably make the most sense here.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

Copy link
Member

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

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

tbh I have not the feeling that you checked every call if it's save to use simple is_file now without cleaning the path.

@Hackwar
Copy link
Member Author

Hackwar commented Mar 14, 2023

tbh I have not the feeling that you checked every call if it's save to use simple is_file now without cleaning the path.

I admit, I did not check these cases, but only checked when the variable was last written to.

@laoneo
Copy link
Member

laoneo commented Mar 14, 2023

Are there any occurrences of File::exists left in the core? If not, then I would add the deprecation message right into this pr.

@Hackwar
Copy link
Member Author

Hackwar commented Mar 14, 2023

No, all occurences have been handled with this. But I would really like to keep the deprecation messages in one PR, among other things because it means I only have to make one PR to the manual repository. If you want me to make 40 PRs to deprecate each method individually, then I would rather defer all that work to you.

@laoneo
Copy link
Member

laoneo commented Mar 14, 2023

You can make one pr in the manual and then reference it from each of these subsequent pr's. Would that be ok?

@Hackwar
Copy link
Member Author

Hackwar commented Mar 14, 2023

Not really, because it would mean that I would have to update #40111 several times for each case where a method was deprecated individually. I also would not simply close #40111, because there are several methods and whole classes which we aren't using at all in core. And it means dozens of PRs poluting our history with something being deprecated here and then something there. And if we don't get all existing code converted in time for 4.4, we wouldn't be able to remove it all in 6.0, just because deprecation is supposed to be done in time with migration of the core. I'm not saying that I wouldn't be able to write all that code in time, but considering that I'm waiting for 2.5 years now for people to test the install progress bar, I have zero confidence in that necessary PRs would be merged in time.

@laoneo
Copy link
Member

laoneo commented Mar 14, 2023

Ok, then leave it as it is.

@SniperSister
Copy link
Contributor

I still would like the https://github.com/orgs/joomla/teams/security-leads to have a look at this.

Looks ok from the security side of things

@Hackwar Hackwar requested a review from HLeithner March 15, 2023 12:21
@Hackwar
Copy link
Member Author

Hackwar commented Mar 15, 2023

All changes have been applied. Ready to test/codereview/merge. 😃

@laoneo laoneo merged commit e62ddd9 into joomla:4.4-dev Mar 15, 2023
@laoneo
Copy link
Member

laoneo commented Mar 15, 2023

Thanks!

@laoneo laoneo added this to the Joomla! 4.4.0 milestone Mar 15, 2023
@Hackwar Hackwar deleted the 4.4-filesystem-file branch April 17, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants