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

Better PHP Support #2432

Open
HighLiuk opened this issue Nov 1, 2023 · 10 comments · May be fixed by #2470
Open

Better PHP Support #2432

HighLiuk opened this issue Nov 1, 2023 · 10 comments · May be fixed by #2470

Comments

@HighLiuk
Copy link

HighLiuk commented Nov 1, 2023

Hey there. First of all, I'd like to thank you all for this tool, it's awesome.

I'd like to use quicktype a lot on PHP, but I've tried it and IMHO I'd never use the generated code like this.
So I've cloned it locally and in a week or so I've managed to write a PHP implementation that suits my needs.
Is there any chance I can contribute to enhance PHP language support? Also, is there any rule to follow in order to have it approved?

Thank you in advance.

@comod
Copy link

comod commented Dec 27, 2023

Please explain what youve customized. I am currently analyzing quicktype especially for php as well

@HighLiuk
Copy link
Author

@comod here you go if you want to test it yourself > https://github.com/HighLiuk/quicktype

I've added support for a lot of different features ranging through PHP 7.3 (or older) to PHP 8.2 which has far way less boilerplate than previous versions of PHP especially for OOP.

I've added a dozen options as shown in this local version of the web app I've scaffolded to play with it. As you can see from the screenshot, choosing which PHP version to target is an option. This internally guarantees that you cannot opt in for features that are not available for that PHP version (e.g. readonly attributes are introduced in PHP 8.1. These may be turned off with a flag (defaults to true for simplicity) but are auto-disabled if the PHP version is lower than 8.1)

It is not 100% accurate (I recently noticed a few inconsistencies and so) and I've not run unit tests against it, but if a PR is accepted I would add the required tests.

Also, I've revised how the validation works in order to keep it simpler.

Let me know what do you think about.
Would be nice if a maintainer read this too (perhaps liking the thread would help?)

@comod
Copy link

comod commented Dec 30, 2023

At first glance, this looks exactly like what I need as well. I have already started building my own version as well. Things I've noticed or changed: enums (BackedEnum), validations which are currently completely pointless. From/To-Array instead of stdClass, Multiple-Files with namespaces and imports instead of a god-class. Let's go into the details in the new year and then we simply create a pull request.

@HighLiuk
Copy link
Author

I saw that other languages (like Go) have support for output multiple files but I didn't figure out how to implement it.
In PHP this is necessary when you leverage autoloads and if you have to do that manually over several generated classes, it easily becomes messy.

@dvdsgl
Copy link
Member

dvdsgl commented Jan 6, 2024

Hello! You can make a PR and we can review it there.

@comod
Copy link

comod commented Jan 9, 2024

i figured out that it needs to remove this line to get it work @HighLiuk:
image
The generated code looks good so far!

I was able to generate multiple files with some few changes like this:
image

but i dont know how to add namespaces and imports. should this be inferred by additional properties in the json schema or should this be inferred by the original structure of the json-schema source. i only get fileName as parameter and not the whole filePath. so this is kind of tricky - any ideas?

@HighLiuk
Copy link
Author

HighLiuk commented Jan 9, 2024

Hey there @comod thanks in advance

but i dont know how to add namespaces and imports. should this be inferred by additional properties in the json schema or should this be inferred by the original structure of the json-schema source. i only get fileName as parameter and not the whole filePath. so this is kind of tricky - any ideas?

probably you should look for the property types to get which classes / enums need to be imported at the top of the file.

But a clever option would be to add a "namespace" option to add a namespace on the top of the file. Like --namespace=Glideapps\Quicktype so for instance class Foo would have namespace Glideapps\Quicktype; on top of the file. This way according to PSR-4 you don't even need to add use Glideapps\Quicktype\Foo; if you add them to the same directory with the same namespace.

@comod
Copy link

comod commented Jan 9, 2024

I wasnt clear enough. What i try to achive is multiple files in different folders either according to the source structure (which is already organized in different folders) or to any additional information from the json schema (like a property named "package" or "namespace") if this makes any sense. Maybe this is out of scope and should be done as extra step

@comod
Copy link

comod commented Jan 11, 2024

I have now delved even deeper into the subject matter and have a few basic questions for the authors:

  • Is there already a concept for namespaces that spread across multiple levels, or are there only "global namespaces", i.e., ultimately a single namespace that is simply appended everywhere?

In the code, for example, I found this:

    private _globalForbiddenNamespace: Namespace | undefined;
    private _otherForbiddenNamespaces: Map<string, Namespace> | undefined;
    private _globalNamespace: Namespace | undefined;

However, I could not understand what this is used for. I believe this has nothing to do with the namespace I mean.

  • Is it possible to add and read a custom top-level property to the JSON schema? What would be the places that would need to be adjusted for this?
  • I have already tried using a custom Namer, but file and class names are inseparably constructed in the code. As a workaround, it would initially suffice to build namespaces based only on the "title" property (e.g., "title": "myNameSpace/myClassName"). File and class name, however, refer to the same Name/Namer, and I am not sure whether I should use these or whether there is not a better way.

Finding a solution outside is no less challenging, as not only namespaces have to be added, but all references also have to be adjusted. Additionally, the sources would have to be read and mapped, etc. It would simply be more elegant to get a grip on this with this one tool on the spot.

If it is to remain with just the one global namespace, then what @HighLiuk suggested is indeed sufficient.

@HighLiuk
Copy link
Author

Not sure if I understand everything @comod, but I agree that it should be simpler to use QuickType's internal API to easily print on different files.

@HighLiuk HighLiuk linked a pull request Jan 11, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants