Skip to content

[Filesystem] Added additional check for temporary file existence on cleanup step #39239

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

Closed

Conversation

gechetspr
Copy link
Contributor

@gechetspr gechetspr commented Nov 30, 2020

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #39235
License MIT
Doc PR N/A

Added additional file existing check before removing temporary file in Filesystem::dumpFile() method.

nicolas-grekas and others added 30 commits October 22, 2020 20:37
…e (nicolas-grekas)

This PR was merged into the 5.1 branch.

Discussion
----------

[TwigBridge] Remove "transchoice" from the code base

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Commits
-------

713c262 [TwigBridge] Remove "transchoice" from the code base
…ient (jderusse)

This PR was merged into the 5.x branch.

Discussion
----------

[HttpClient] Add a Stopwatch on TraceableHttpClient

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

I used this code to generate the screenshot below.
```php
$response = $client->request('GET', 'https://httpstat.us/200');
$response->getContent();

$responses[] = $client->request('GET', 'https://httpstat.us/200?sleep=300');
$responses[] = $client->request('GET', 'https://httpstat.us/200?sleep=100');
foreach ($client->stream($responses) as $chunk) {
}
```
![Screenshot from 2020-10-23 02-04-43](https://user-images.githubusercontent.com/578547/96942031-41883580-14d4-11eb-848e-fd21d2f4fec9.png)

Commits
-------

e6f6b4c Add Stopwatch on TraceableClient
…changelog (jschaedl)

This PR was merged into the 5.x branch.

Discussion
----------

[Notifier] Add missing upgrade entries and fixed changelog

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       |  <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | <!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch 5.x.
-->

Follow-up of symfony#35773

Commits
-------

c6b32cd add missing upgrade entries and fixed changelog
…ory (Nyholm)

This PR was squashed before being merged into the 5.x branch.

Discussion
----------

[RateLimiter] Rename RateLimiter to RateLimiterFactory

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | no
| Deprecations? | No, not released yet
| Tickets       |
| License       | MIT
| Doc PR        | should be added

Sorry for making a few BC breaks.

@wouterj [said](symfony#38562 (comment)) that this class was suggested to be named `LimiterFactory` before. But that was rejected.

Just my looking at the names of the classes we currently have:
- Rate
- RateLimit
- RateLimiter

I find it hard to know what these are doing and the difference between them. Note that none of them are used as a rate limiter (ie implements `LimiterInterface`)

I would like to be clear that a `RateLimiterFactory` is used to create an object implementing `LimiterInterface`.

Commits
-------

8be261b [RateLimiter] Rename RateLimiter to RateLimiterFactory
…yholm)

This PR was merged into the 5.x branch.

Discussion
----------

[HttpClient] Adding missing dependency for dev

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

This will make sure Travis is happy again.

Commits
-------

552e704 [HttpClient] Adding missing dependency for dev
* 4.4:
  fix merge
  Remove branch-version (keep them for contracts only)
  [HttpClient] relax auth bearer format requirements
  [PHPUnitBridge] Silence errors from mkdir()
  [DependencyInjection] Preload classes with union types correctly.
  [Serializer] fix decoding float XML attributes starting with 0
  add missing dutch translations
  Support PHPUnit 8 and PHPUnit 9 in constraint compatibility trait
  Add expectDeprecation, expectNotice, expectWarning, and expectError to TestCase polyfill
  Add missing exporter function for PHPUnit 7
  [Validator] Add missing romanian translations
  [Cache] Use correct expiry in ChainAdapter
  do not translate null placeholders or titles
* 5.1:
  fix merge
  fix merge
  Remove branch-version (keep them for contracts only)
  [HttpClient] relax auth bearer format requirements
  [PHPUnitBridge] Silence errors from mkdir()
  [DependencyInjection] Preload classes with union types correctly.
  [Serializer] fix decoding float XML attributes starting with 0
  add missing dutch translations
  [TwigBridge] Remove "transchoice" from the code base
  Support PHPUnit 8 and PHPUnit 9 in constraint compatibility trait
  Add expectDeprecation, expectNotice, expectWarning, and expectError to TestCase polyfill
  [String] fix before/after[Last]() returning the empty string instead of the original one on non-match
  Add missing exporter function for PHPUnit 7
  [Validator] Add missing romanian translations
  [String] fix slicing in UnicodeString
  [Cache] Use correct expiry in ChainAdapter
  do not translate null placeholders or titles
* 4.4:
  Disable platform checks
  Put branch-version in the source for CI
  Bump default PHPUnit version for PHP 8 to 9.4.
* 5.1:
  Disable platform checks
  Put branch-version in the source for CI
  Bump default PHPUnit version for PHP 8 to 9.4.
…rInterface to a new namespace (Nyholm)

This PR was squashed before being merged into the 5.2-dev branch.

Discussion
----------

[RateLimiter] Moved classes implementing LimiterInterface to a new namespace

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no?
| Tickets       |
| License       | MIT
| Doc PR        |

Before we release the RateLimit component.
I think it would be a good idea to put the 7 classes that belongs to a specific strategy in their own "Policy" namespace. It is very likely that it will be more strategies in the future and the `Symfony\Component\RateLimiter` namespace is crowed as it is.

I decided not to put the `CompoundLimiter` in this namespace as it is not a strategy.

Commits
-------

1e6cea5 [RateLimiter] Moved classes implementing LimiterInterface to a new namespace
* 4.4:
  [Cache] Fixed broken test
* 5.1:
  [Cache] Fixed broken test
This PR was merged into the 5.2-dev branch.

Discussion
----------

[Form] Added missing German translations

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Part of symfony#38710
| License       | MIT
| Doc PR        | N/A

Commits
-------

37a2ff4 [Form] Added missing German translations.
…henticator (chalasr)

This PR was merged into the 5.1 branch.

Discussion
----------

[Security] Remove dead references to AnonymousAuthenticator

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Commits
-------

9376e87 [Security] Remove dead references to AnonymousAuthenticator
fabpot and others added 8 commits November 29, 2020 10:27
* 4.4:
  Bump Symfony version to 4.4.18
  Update VERSION for 4.4.17
  Update CHANGELOG for 4.4.17
* 5.1:
  Bump Symfony version to 5.1.10
  Update VERSION for 5.1.9
  Update CHANGELOG for 5.1.9
  Bump Symfony version to 4.4.18
  Update VERSION for 4.4.17
  Update CHANGELOG for 4.4.17
* 5.2:
  Bump Symfony version to 5.1.10
  Update VERSION for 5.1.9
  Update CHANGELOG for 5.1.9
  Bump Symfony version to 4.4.18
  Update VERSION for 4.4.17
  Update CHANGELOG for 4.4.17
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@gechetspr gechetspr changed the title Added additional check for temporary file existence on cleanup step [Filesystem] Added additional check for temporary file existence on cleanup step Nov 30, 2020
@gechetspr
Copy link
Contributor Author

#39235

@derrabus
Copy link
Member

Can you please rebase your change on the lowest maintained branch the problem occurs on? The linked issue suggests that the problem happens on 5.1, so we should at least fix it on 5.1. If it happens on 4.4 as well, we need to fix it on 4.4.

@derrabus derrabus added this to the 5.1 milestone Nov 30, 2020
@gechetspr gechetspr changed the base branch from 5.x to 4.4 November 30, 2020 12:55
@gechetspr
Copy link
Contributor Author

I'll reopen a new one, sorry

@gechetspr gechetspr closed this Nov 30, 2020
@jderusse
Copy link
Member

jderusse commented Nov 30, 2020

This should target branch 4.4 as it has been introduced by #39059

@gechetspr
Copy link
Contributor Author

#39243

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.