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 missing allowed types to constructor (#239) #245

Closed
wants to merge 2 commits into from

Conversation

ppaulis
Copy link

@ppaulis ppaulis commented Mar 18, 2024

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

This bugfix addresses the issue #239 by allowing lists in the constructor of ViewModel.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Closing as invalid: these types are too wide, even if the more internal setVariables() is not as restrictive

@@ -76,7 +76,7 @@ class ViewModel implements ModelInterface, ClearableModelInterface, RetrievableC
/**
* Constructor
*
* @param null|array<string, mixed>|Traversable<string, mixed>|ArrayAccess<string, mixed> $variables
* @param null|array<string, mixed>|Traversable<string, mixed>|ArrayAccess<string, mixed>|array<int, mixed>|Traversable<int, mixed>|ArrayAccess<int, mixed> $variables
Copy link
Member

Choose a reason for hiding this comment

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

I'd say that all the int-keyed values should be rejected.

Yes, it works in practice: no, it shouldn't be allowed, and there's no guarantee that it will.

Copy link
Author

Choose a reason for hiding this comment

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

@Ocramius Thanks for the quick reply :-)
So, to be sure that I understand you correctly : If I want to return in my Laminas Api Tools RPC Controller something like :

[
  {
    "field1": "value1",
    "field2": "value2"
  },
  {
    ..
  },
  ..
]

JsonModel/ViewModel shouldn't be used?

Copy link
Member

Choose a reason for hiding this comment

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

ViewModel: probably not.
JsonModel: yes, should be OK 🤔

Copy link
Author

Choose a reason for hiding this comment

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

@Ocramius ok, do you have a suggestion of how to implement this?
I see a few possibilities here, but ultimately it leads me to thinking that JsonModel shouldn't inherit from ViewModel and that ModelInterface contains a lot of stuff that is not necessary for JsonModel...

Copy link
Member

Choose a reason for hiding this comment

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

@ppaulis JsonModel could declare its own __construct, with more types (mixed comes to mind, to be honest)

Copy link
Author

Choose a reason for hiding this comment

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

@Ocramius so we continue relying on the same code with new tests for JsonModel specifically (haven't deep-dived into the existing tests yet), testing the discussed JSON format. And when the day comes that the ViewModel would no longer support integer-indexes, we would need to think about how to keep the JsonModel running.

if that's ok, I'll send you over a new PR until end of the week.

Copy link
Member

Choose a reason for hiding this comment

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

@gsteel
What are you planning for version 3.0 on this?

Copy link
Member

Choose a reason for hiding this comment

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

Rendering Strategies are, IMO, MVC specific - it's been a while since I had time to allocate to view, but I think my initial plan was to move the rendering strategies to the as yet un-released laminas-mvc-view component.

@Ocramius Ocramius closed this Mar 18, 2024
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 this pull request may close these issues.

4 participants