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

Added required public methods to the DataBuilderInterface #586

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

danielmorell
Copy link
Collaborator

Description of the change

The first major update is adding public methods to DataBuilderInterface that are called during the normal course of operation. This is evidenced by the fact that the test fixture FakeDataBuilder included more methods than the interface required. If those methods were removed, the tests would start throwing errors because methods were called that did not exist.

This is important because it is possible for a user to do something like the following...

Rollbar::init(['dataBuilder' => MyCustomDataBuilder::class]);

The dataBuilder config option is not readily apparent in the code and is not in our developer documentation. (I didn't realize it existed until a few months ago.)

If a user supplies their own data builder class, it must implement the DataBuilderInterface. However, if the user only implemented the methods that are required by the interface, we would start throwing exceptions when methods are called that don't exist. This honestly seems like a bit of an edge case, since rewriting the entire DataBuilder class seems unlikely. Most people would extend the DataBuilder class.

The reason this is being included is because fixing it is a breaking change. Otherwise, it seems like a minor issue.

The second major change is removing the tests around custom data that is not an array. Our entire logic around custom data assumes the 'custom' config key is an array. In March of this year, I added these tests to ensure primitives and different objects would work correctly. At the time I was not as aware of the rest of our codebase that deals with the custom data array. Interestingly these tests passed. However, trying to use something other than an array would cause issues elsewhere.

I also updated the logic in DataBuilder::addCustom() to remove the need to throw an exception.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Related issues

None

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

@danielmorell danielmorell added the Type: Maintenance General up-keep, or changes that tidy an existing component or process. label Dec 19, 2022
@danielmorell danielmorell added this to the v4.0.0 milestone Dec 19, 2022
Copy link
Contributor

@waltjones waltjones left a comment

Choose a reason for hiding this comment

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

@danielmorell thank you for the thoughtful description and PR.

@danielmorell danielmorell merged commit f0f665d into next/4.0/main Dec 20, 2022
@danielmorell danielmorell deleted the 4.0/data-builder-interface branch December 20, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance General up-keep, or changes that tidy an existing component or process.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants