[BUGFIX] Ensure temp directory exists before calling tempnam() + DI support #1279
Conversation
CybotTM
force-pushed
the
bugfix/plantuml-tempnam-notice
branch
from
2cc375c to
eae4d50
Compare
packages/guides-graphs/src/Graphs/Renderer/PlantumlRenderer.php
Outdated
Show resolved
Hide resolved
| PUML; | ||
|
|
||
| $pumlFileLocation = tempnam(sys_get_temp_dir() . '/phpdocumentor', 'pu_'); | ||
| $tempDir = sys_get_temp_dir() . self::TEMP_SUBDIRECTORY; |
There was a problem hiding this comment.
Maybe it's better to inject the directory used by this class, I remember when I wrote this I didn't write a test because it was hard to run this in isolation. Injecting the path can be done via the constructor and would allow us to use vfsStream to fake the filesystem.
There was a problem hiding this comment.
Good suggestion for testability! This was initially out of scope for the bugfix, but I can include it if you'd like. Let me know.
There was a problem hiding this comment.
If you can do that, that would be great.
Please keep in mind that we do not want to be backwards compatible. So any new constructor argument must have a default value. Which could still be sys_get_tmp_dir.
There was a problem hiding this comment.
If you can do that, that would be great.
Please keep in mind that we do not want to be backwards compatible. So any new constructor argument must have a default value. Which could still be sys_get_tmp_dir.
There was a problem hiding this comment.
Please keep in mind that we do not want to be backwards compatible.
I assume a typo here, you want to be BC, right? Otherwise the new constructor argument would not need to have a default value.
CybotTM
force-pushed
the
bugfix/plantuml-tempnam-notice
branch
2 times, most recently
from
8105982 to
e6a7378
Compare
The tempnam() call uses a subdirectory of sys_get_temp_dir() that may not exist, which triggers a PHP E_NOTICE when the directory is missing. PHP then falls back to the system temp directory, but the notice can appear in rendered output. This creates the directory if it doesn't exist before calling tempnam(), preventing the notice. Also extracts the temp subdirectory path to a public constant so tests can reference it directly, ensuring they stay in sync with the implementation.
Adds a unit test to verify that the temp directory is created before calling tempnam(), preventing the E_NOTICE regression.
…erer Allow temp directory to be injected via constructor for better testability and flexibility. Falls back to system temp directory with /phpdocumentor subdirectory if not provided.
CybotTM
force-pushed
the
bugfix/plantuml-tempnam-notice
branch
from
e6a7378 to
a40aec7
Compare
CybotTM
changed the title
|
Thanks for your help. |
2 participants
Summary
The
tempnam()call inPlantumlRenderer::render()uses a subdirectory ofsys_get_temp_dir()that may not exist, which triggers a PHPE_NOTICE:This notice can appear in rendered documentation output when error reporting includes
E_NOTICE.Changes
tempnam()sys_get_temp_dir() . '/phpdocumentor')Test plan
renderer="plantuml")/tmp/phpdocumentordoes not exist.. uml::directiveE_NOTICEis emittedMoved from phpDocumentor/guides-graphs#2