[7.x] Remove wasted file read when loading package manifest

[TheNodi]

This PR removes an unused file_get_contents of the packages manifest file.

How / Why
I was optimizing an application to have zero disk accesses, but I noticed the package manifest was read on every request, even though it is a PHP file and it should be cached by opcache.

I dug into the code and I find out there's a get of the manifest file before requiring it:

framework/src/Illuminate/Foundation/PackageManifest.php

Line 109 in 407372d

$this->files->get($this->manifestPath);

This translates to always reading the file into memory, and then throwing away the result.

$this->files->get was introduced in commit 683637a as a locking mechanism to prevent partial reads of the manifest file.
In commit 633a907 the locking was substituted by an atomic rename-based file write. The get called survived the refactor, resulting a no-op file_get_contents that just trashes the output.

Behavior Differences
When the build function is not capable of creating the file, the filesystem get would have thrown a FileNotFoundException. After this changes, an empty manifest is returned instead.
I don't think there is a way for build to silently fail a write, and the following line is explicitly checking the existence of the file. Hence I believe this is the intended behavior.

[GrahamCampbell]

I don't think we can merge this. It looks like it was added as a bug fix: #25012.

[TheNodi]

At the time of #25012, the manifest was written using file_put_contents without locking, which can result in multiple concurrent writes.

As of #26010, the write is replaced by Filesystem::replace:

• Create a temp file with the content we want
• Rename the temp file to the target manifest location

Because rename is atomic in Linux, there are no concurrent writes anymore. The last rename sets the final file contents.

In fact, the locking is not used anymore (removed by #26010). The author removed the locking instead of removing the function call by mistake.

[GrahamCampbell]

Right, thanks for those details.

[GrahamCampbell]

After this changes, an empty manifest is returned instead.

I think we still need to throw an exception?

[GrahamCampbell]

Doesn't getRequire raise an error or throw an exception, or something, when not found?

[TheNodi]

I just realized that, yes.

Should we simplify the call to the following?

return $this->manifest = $this->files->getRequire($this->manifestPath);

[GrahamCampbell]

No, I think this is fine as is.

[GrahamCampbell]

In fact, this PR should possibly go to 6.x instead of 7.x.

[TheNodi]

Sure, I'm opening a PR from / to 6.x branch.

[driesvints]

Closing this in favor for the other PR. @TheNodi you might wanna transfer your description to it.