SM-1402 - review and update php sdk #1032
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit d124c37.
|
New Issues
Fixed Issues
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
==========================================
- Coverage 58.18% 58.16% -0.03%
==========================================
Files 197 197
Lines 13487 13523 +36
==========================================
+ Hits 7847 7865 +18
- Misses 5640 5658 +18 ☔ View full report in Codecov by Sentry. |
|
The #878 have conflicting changes. Maybe you join them together or pick changes individually from other one, so we won't have to redu the testing ? |
mzieniukbw
previously approved these changes
Sep 11, 2024
coltonhurst
requested changes
Sep 12, 2024
languages/php/INSTALL.md
Outdated
|
|
||
| ## Introduction | ||
|
|
||
| Composer is used to build PHP Bitwarden client library. |
There was a problem hiding this comment.
Suggested change
| Composer is used to build PHP Bitwarden client library. | |
| Composer is used to build the PHP Bitwarden client library. |
languages/php/INSTALL.md
Outdated
| - Linux x86_64: `src/lib/linux-x64/libbitwarden_c.so` | ||
| - macOS x86_64: `src/lib/macos-x64/libbitwarden_c.dylib` | ||
| - macOS aarch64: `src/lib/macos-arm64/libbitwarden_c.dylib` | ||
| - If you prefer to build SDK yourself, see [SDK README.md](../../README.md) for instructions. |
There was a problem hiding this comment.
Suggested change
| - If you prefer to build SDK yourself, see [SDK README.md](../../README.md) for instructions. | |
| - If you prefer to build the SDK yourself, see the [SDK README.md](../../README.md) for instructions. |
languages/php/README.md
Outdated
| PHP bindings for interacting with the [Bitwarden Secrets Manager]. This is a beta release and might be missing some functionality. | ||
| Supported are CRUD operations on project and secret entities. | ||
| PHP bindings for interacting with the [Bitwarden Secrets Manager]. This is a beta release and might be missing some | ||
| functionality. |
There was a problem hiding this comment.
Any reason this is on a new line?
There was a problem hiding this comment.
Not sure. I'll fix it.
languages/php/README.md
Outdated
| $identity_url = "<identity url>"; | ||
| require_once 'vendor/autoload.php'; | ||
|
|
||
| $access_token = '<access-token>'; |
There was a problem hiding this comment.
Suggested change
| $access_token = '<access-token>'; | |
| $access_token = "<access-token>"; |
Just a nit, as the others use "
coltonhurst
previously approved these changes
Sep 12, 2024
mzieniukbw
reviewed
Sep 13, 2024
mzieniukbw
requested changes
Sep 13, 2024
coltonhurst
previously approved these changes
Sep 13, 2024
mzieniukbw
previously approved these changes
Sep 13, 2024
dani-garcia
previously approved these changes
Sep 18, 2024
## 🎟️ Tracking https://bitwarden.atlassian.net/browse/SM-1402 (Not part of this ticket, but agreed with @tangowithfoxtrot that it would make sense to do so) ## 📔 Objective Automatic PHP Schema generation using quicktype. The schema is not compatible with previously generated Swaggest based schema - PHP code changes are required (does not affect the usage, `README.md` and `example.php` works as before) Notable code changes: - Schema class fields are private, no setters or getters. Can only init the classes via constructor. (Could enable getters in schema generation, but the quicktype does not respond well with nested arrays that are part of schema, as a result PHP generated schema file does not compile) - Schema class constructors are initialized with named arguments, which is more readable than `new Command(null, null, null, null, null, $secrets_command, null, null) - Uses PHP native json serialization, but SDK does not react well to null fields, so adjusted the code to remove it just before JSON serialization. - Separated `AuthClient` into separate file - Imports with `uses` ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or⚠️ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes
mzieniukbw
dismissed stale reviews from dani-garcia, coltonhurst, and themself
via
49b5023
coltonhurst
reviewed
Sep 18, 2024
This reverts commit a57e2ed. Access token link was already there.
coltonhurst
approved these changes
Sep 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/SM-1402
📔 Objective
Update PHP bindings in accordance with our other wrappers. This renames any "put" methods to "update", refactors
access_token_logintoauth().login_access_token, re-orders function args forcreateandupdate, and adds secret syncing.This update required quite a few changes to the schemas. However, since we cannot auto-generate them with
quicktype(see the error referenced in glideapps/quicktype/pull/2407), schemas were generated with the swaggest/json-cli:json-cli gen-php ../../support/schemas/schema_types/SchemaTypes.json --ns '\Bitwarden\Sdk\Schemas' --ns-path ./src/schemas/The generated schemas still required hand modification to get human-readable class names for things like
ProjectCommand,SecretCommand, etc.To validate the changes, I've run the
example.phpfile after updating the schemas.⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes