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

Revert undefined property declarations #453

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

onlime
Copy link

@onlime onlime commented Oct 15, 2024

This reverts 278bdac (released in 1.24.1), which introduced explicit properties which were previously accessed as magic props through __get().

This broke e.g. Zend_Pdf_Resource_Image_Png#L287, where $decodingStream->value was always null:

                $pngDataRawDecoded = $decodingStream->value;

while previously (1.23.5) the correct binary image data was accessed through $decodingStream->_value->value, e.g. on such a sample object (using Laravel's dd()):

Zend_Pdf_Element_Object_Stream^ {#53160
  +value: null
  -_parentObject: Zend_Pdf_Element_Object_Stream^ {#53160}
  #_value: Zend_Pdf_Element_Stream^ {#56323
    +value: Zend_Memory_Container_Locked^ {#53621
      +value: b"\x18***********************************************"
    }
    -_parentObject: null
  }
...

I hope such breaking changes were not introduced in other commits.

This PR fixes the instantiation of Zend_Pdf_Image::imageWithPath($path) - couldn't test the other 3 occurrences where @Fredthelead has introduced similar props in #438 Support for PHP 8.2

@sreichel
Copy link

This broke e.g. Zend_Pdf_Resource_Image_Png#L287, where $decodingStream->value was always null:

Not tested, but setting the properties to null should work too? (eg public $value = null)

Copy link

@Fredthelead Fredthelead left a comment

Choose a reason for hiding this comment

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

I completely missed this, thanks for letting me know!

@onlime
Copy link
Author

onlime commented Oct 16, 2024

This broke e.g. Zend_Pdf_Resource_Image_Png#L287, where $decodingStream->value was always null:

Not tested, but setting the properties to null should work too? (eg public $value = null)

Tested, it would break it as well, as the mentioned code actually accesses $this->_value->value and the explicitly defined prop $this->value would override it. It would never get set, at least not in that use case.

@develart-projects
Copy link
Collaborator

Two notes here:

  1. are we going to revert whole change, when we have only one error confirmed?

  2. maybe it's a good idea to extend UnitTest for this particular problem, as we have the error confirmed?

PS: anyway, I'm voting for capital punishment to the developer of this class.

@IMSoP
Copy link

IMSoP commented Oct 18, 2024

The cause of the break here is that there are classes inheriting from Zend_Pdf_Element which define __get in order to have dynamic properties. Declaring a real property with that name caused it to no longer call that magic method.

The other three classes in the original commit are much more specific, and don't suffer the same problem, so it's only that one class we need to be concerned with.

Regarding the Zend_Pdf_Element case, it looks like:

  • Zend_Pdf_Element itself only accesses $this->value in toPhp()
  • these classes write to $this->value, but also declare it themselves: Zend_Pdf_Element_Boolean, Zend_Pdf_Element_Name, Zend_Pdf_Element_Numeric, Zend_Pdf_Element_String, Zend_Pdf_Element_String_Binary, Zend_Pdf_Element_Null, Zend_Pdf_Element_Stream
  • these classes define a magic __get and __set, so might or might not be dynamically defining $this->value somewhere: Zend_Pdf_Element_Reference, Zend_Pdf_Element_Dictionary, Zend_Pdf_Element_Object, Zend_Pdf_Element_Array, Zend_Pdf_Element_Object_Stream

That's a pretty confusing mix, and all the solutions I thought of break one child class or another. I guess the safest for now is to revert the change to that file (but not the whole commit).

@develart-projects
Copy link
Collaborator

@IMSoP agreee with this, so @onlime pls update the PR to revert only the affected class, thanks.

@onlime
Copy link
Author

onlime commented Oct 18, 2024

Thanks a lot @IMSoP for your investigation and for describing this confusing mess in such detail. I have not reverted the other changes and only applied it to Zend_Pdf_Element in 38230d5

PHPStan should also be happy with the @property tag declaration.

@develart-projects develart-projects added bug Something isn't working to be released PR exists or in master, but not released yet labels Oct 18, 2024
@develart-projects develart-projects added this to the 1.24.2 milestone Oct 18, 2024
@develart-projects develart-projects merged commit d7529e9 into Shardj:master Oct 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working to be released PR exists or in master, but not released yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants