Closed
Conversation
Archon's object composition technique makes heavy use of a PHP feature allowing method calls from an incompatible context. However, this ability was deprecated in PHP 5.6 and removed from PHP 7.0 [1]. Ultimately, it seems like moving over to Traits may be a logical path forward. This patch instead attempts to allow current Archon to run on PHP 7+ with the fewest changes possible. Rather than simply attempt the method call in the wrong context, we instead create a closure in the desired class, then insert our current context into that closure using bindTo. There may, of course, be a simpler way to do this. In particular, it seems messy to need to create an object only to get the closure. I do not claim to be an expert in PHP internals, but this functions, and is decently boiled down. This new technique should be supported for PHP >= 5.4.0, but this is untested. One questionable area would be the use of the getClosure method call on a parenthesized "new" object statement. I believe it is supported syntax on at least 5.6, and would be very easy to work around in any case. That said, it may be better to retain the old technique and branch within the code to suit the running version. [1] http://php.net/manual/en/migration56.deprecated.php#migration56.deprecated.incompatible-context Signed-off-by: Dan Wells <dbw2@calvin.edu>
$OutputQueries is not static, so we need $this here. Older versions of PHP (<7.0) were more lax about this, particular the way we were mixing contexts. Signed-off-by: Dan Wells <dbw2@calvin.edu>
PHP no longer supports "<?" as an opening tag; make it read "<?php" instead. Signed-off-by: Dan Wells <dbw2@calvin.edu>
|
This is great! Did you also test this code against 5.6 to make sure it maintains compatibility with that version of php?
Chris Prom
… On May 18, 2017, at 4:01 PM, Hekman Library Technology Team ***@***.***> wrote:
We are in the process of adopting Archon at Hekman Library, Calvin College. After getting it up an running on a PHP 5.6 test server, we then ran into some compatibility issues with PHP 7 on our production server.
The contents of this pullrequest got us to a working state. There may be more changes lurking in areas of the code we haven't exercised (similar to the small tag fix in the EAD export, included here), but we are now able to browse, search, and do DB imports successfully on our production server.
See the commit comments for much more detail on these changes, and why they are needed.
You can view, comment on, or merge this pull request online at:
#9 <#9>
Commit Summary
Workaround for PHP 7 Compatibility
Fix non-static variable access
Add proper PHP tag
File Changes
M packages/collections/pub/eadlist.php <https://github.com/LibraryHost/archon/pull/9/files#diff-0> (2)
M packages/core/lib/archon.inc.php <https://github.com/LibraryHost/archon/pull/9/files#diff-1> (8)
M packages/core/lib/archonobject.inc.php <https://github.com/LibraryHost/archon/pull/9/files#diff-2> (30)
M packages/core/lib/querylog.inc.php <https://github.com/LibraryHost/archon/pull/9/files#diff-3> (2)
Patch Links:
https://github.com/LibraryHost/archon/pull/9.patch <https://github.com/LibraryHost/archon/pull/9.patch>
https://github.com/LibraryHost/archon/pull/9.diff <https://github.com/LibraryHost/archon/pull/9.diff>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#9>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABMJag_qff04r5-XpHC8eB-kfaAeguJ-ks5r7LGcgaJpZM4Nfynv>.
|
Author
|
Chris, thank you for the quick response. I mostly wanted to get this out there as a place to start. There are a few details regarding PHP 5.6 in the first commit message (summary: it should work), but we haven't been able to test it yet. I'll try to find time to do so in the next day or two and report back. |
|
Hi Heckman Library,
Your message and a few others caused me to finish assessing the Library Host code and integrate a few other changes. At this point, I am planning to release an archon version 3.3 that incorporates the Archon Stability code developed by LibraryHost. This code will should run conversions 5.3 through 5.6 of PHP and was merged with archonproject#78 <archonproject#78>
For that reason, you may want to discuss the best process for moving forward with the code that is PHP 7 compatible, but I would suggest that it should be assessed as a separate branch or fork of the master repo (https://github.com/archonproject/archon <https://github.com/archonproject/archon>), so that eventually it could be assessed for a separate release. Feel free to email or call me directly if you want to discuss this further or think it might work to proceed in a different way.
Thanks!
Chris Prom
University of Illinois Archives
… On May 19, 2017, at 8:31 AM, Hekman Library Technology Team ***@***.***> wrote:
Chris, thank you for the quick response. I mostly wanted to get this out there as a place to start. There are a few details regarding PHP 5.6 in the first commit message (summary: it should work), but we haven't been able to test it yet. I'll try to find time to do so in the next day or two and report back.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABMJavi_3Wpm9JU0Pevm5rJ3-rkcZxZuks5r7ZmtgaJpZM4Nfynv>.
|
Author
|
Chris, thanks again for the feedback. I am not terribly familiar with which branches/forks are active, and how the project is moving. Based on what you said, does it make sense to now open a pullrequest for this on archonproject/archon instead? And is that the main repo to track for following this project in the future? |
Author
|
Closing in favor of archonproject#82 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
We are in the process of adopting Archon at Hekman Library, Calvin College. After getting it up an running on a PHP 5.6 test server, we then ran into some compatibility issues with PHP 7 on our production server.
The contents of this pullrequest got us to a working state. There may be more changes lurking in areas of the code we haven't exercised (similar to the small tag fix in the EAD export, included here), but we are now able to browse, search, and do DB imports successfully on our production server.
See the commit comments for much more detail on these changes, and why they are needed.