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 Generator not generating namespaces to the written classes #98

Closed
wants to merge 1 commit into from
Closed

Fix Generator not generating namespaces to the written classes #98

wants to merge 1 commit into from

Conversation

trm42
Copy link

@trm42 trm42 commented Nov 15, 2023

Hi,

Noticed with (tested with 0.29.2 and current master) that generating classes doesn't work correctly as it doesn't add namespaces as expected. When tested through \Spawnia\Sailor\Tests\Integration\CodegenTest they seem to be generated correctly but with the CLI command ./vendor/bin/sailor namespaces doesn't get added to the generated classes.

This can be reproduced with examples/simple at least:

$ ./test.sh                                                                                                                                        0484160
+ composer update
> rsync -r ../../ sailor --exclude examples --exclude vendor --exclude .idea --exclude .git --exclude .build --delete
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 1 update, 0 removals
  - Upgrading spawnia/sailor (dev-master => dev-04841609dc53da015ae48b0525926d90287b27ef)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 1 update, 0 removals
  - Upgrading spawnia/sailor (dev-master => dev-04841609dc53da015ae48b0525926d90287b27ef): Mirroring from ./sailor
Generating autoload files
10 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
No security vulnerability advisories found.
+ composer reinstall spawnia/sailor
> rsync -r ../../ sailor --exclude examples --exclude vendor --exclude .idea --exclude .git --exclude .build --delete
  - Removing spawnia/sailor (dev-04841609dc53da015ae48b0525926d90287b27ef)
  - Installing spawnia/sailor (dev-04841609dc53da015ae48b0525926d90287b27ef): Mirroring from ./sailor
+ vendor/bin/sailor
Generating code for endpoint simple...
Successfully generated code, query ahead!
+ php src/test.php
PHP Fatal error:  Uncaught Error: Class "Spawnia\Sailor\Simple\Operations\MyObjectQuery" not found in sailor/examples/simple/src/test.php:5
Stack trace:
#0 {main}
  thrown in sailor/examples/simple/src/test.php on line 5

Fatal error: Uncaught Error: Class "Spawnia\Sailor\Simple\Operations\MyObjectQuery" not found in sailor/examples/simple/src/test.php on line 5

Error: Class "Spawnia\Sailor\Simple\Operations\MyObjectQuery" not found in sailor/examples/simple/src/test.php on line 5

Call Stack:
    0.0010     467440   1. {main}() sailor/examples/simple/src/test.php:0

With the changes in the PR, the simple example gets generated with the test.sh correctly and it also does work in the integration tests.

So I changed Generator::asPhpFile method to have additional namespace fallback argument so that the namespace gets generated and set correctly in every possible situation.

The original code works in the integration tests but for example in real life and in the in the examples/simple it fails to generate namespaces to the files.

I haven't yet been able to pinpoint what causes the discrepancy between the integration tests and running the examples/tests manually but did notice the issue when trying to this package in a real world project.

Any ideas, how to trigger the situation in the tests? Will improve the PR if I can find way to reproduce it in the integration tests

  • Added automated tests
  • Documented for all relevant versions
  • Updated the changelog

Changes

Breaking changes

…ack argument so that the namespace gets generated and set correctly in every possible situation. Earlier way does work in the integration tests but for example in real life and in the in the examples/simple it fails to generate namespaces to the files
@spawnia spawnia added the bug An error within Sailor label Nov 15, 2023
{
$printer = new PsrPrinter();
$phpNamespace = $classType->getNamespace();
$class = $printer->printClass($classType, $phpNamespace);

$outputNamespace = (string) $phpNamespace;
Copy link
Owner

Choose a reason for hiding this comment

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

Let's not rely on magic __toString().

{
$printer = new PsrPrinter();
$phpNamespace = $classType->getNamespace();
$class = $printer->printClass($classType, $phpNamespace);

$outputNamespace = (string) $phpNamespace;
if (empty(trim($outputNamespace))) {
Copy link
Owner

Choose a reason for hiding this comment

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

Compare with null or ''.

{
$printer = new PsrPrinter();
$phpNamespace = $classType->getNamespace();
$class = $printer->printClass($classType, $phpNamespace);

$outputNamespace = (string) $phpNamespace;
Copy link
Owner

Choose a reason for hiding this comment

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

This workaround needs a healthy, long code comment to explain its purpose.

@spawnia
Copy link
Owner

spawnia commented Nov 15, 2023

I just experienced this issue in a project of mine as well after updating its dependencies with composer update. nette/php-generator was bumped from 4.1.0 to 4.1.2. I found that their version 4.1.1 broke things.

@trm42
Copy link
Author

trm42 commented Nov 16, 2023

@spawnia Awesome that the real culprit was found, thank you 🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error within Sailor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants