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

Update to String package 1.3 #6600

Closed
wants to merge 3 commits into from
Closed

Update to String package 1.3 #6600

wants to merge 3 commits into from

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Mar 28, 2015

One of the changes in PHP 7 is the platform now allows Scalar Type Hints. Implementing this change means there are now some new reserved keywords in the language that cannot be used for class names, such as "string". We have a class named just that in the Framework's String package and have managed its deprecation at that level.

This PR updates the CMS to use the latest String package which includes a new "StringHelper" class to replace the existing "String" class. JString is modified to extend this directly versus the older class.

Testing Instructions

JString is used to help create aliases on items when you save them. So you should be able to create a new item and validate the alias is still processed correctly.

B/C Implication

As JString, String, and StringHelper are all abstract, there is no practical reason to typehint against this class. However, in the very off chance someone may be, a JString object will no longer match a typehint for a String class. The typehint can be updated for StringHelper and everything will work out OK. I realize this is a fat chance, but it is a B/C break and the options are either this or Joomla 3 will not function on PHP 7.

@wilsonge wilsonge added this to the Joomla! 3.5.0 milestone Mar 31, 2015
@mbabker
Copy link
Contributor Author

mbabker commented Mar 31, 2015

PR is updated to include the utilities and registry packages as well due to their use of the string package.

@mbabker
Copy link
Contributor Author

mbabker commented Mar 31, 2015

Also added the application package to this because it had a reference to String that was replaced. And dropped unit tests fully for the registry and string packages (something about the string tests was causing the bad class to trigger in PHP 7 and the CMS shouldn't need to run a now partial test suite on code it is consuming).

Conflicts:
	tests/unit/suites/libraries/joomla/string/TestHelpers/JString-helper-dataset.php
@mbabker
Copy link
Contributor Author

mbabker commented May 14, 2015

If this is pegged for 3.5, can it be merged to the 3.5 branch at some point? PHPUnit won't even run on PHP 7 right now because of the String class, so basically everything is stalled out trying to work out PHP 7 compatibility.

@wilsonge
Copy link
Contributor

I aim to merge this tonight

@wilsonge wilsonge self-assigned this May 14, 2015
@nikosdion
Copy link
Contributor

@test I can confirm the patch. Before the patch: PHP 7 blew up telling me that string is a reserved class name. After the patch: I get the installer page. It won't go anywhere after that (loops back to the same page), but it's a good start!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6600.

wilsonge added a commit that referenced this pull request May 14, 2015
@wilsonge
Copy link
Contributor

Merged in 834ad2c

@wilsonge wilsonge closed this May 14, 2015
@mbabker mbabker deleted the String13 branch May 14, 2015 18:29
@mbabker
Copy link
Contributor Author

mbabker commented May 14, 2015

👍

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.

None yet

4 participants