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

Fixed issue #73 #74

Merged
merged 1 commit into from
Nov 10, 2014
Merged

Fixed issue #73 #74

merged 1 commit into from
Nov 10, 2014

Conversation

mkroetzsch
Copy link
Contributor

Escape text type properties in icalendar as recommended in the RFC. Tested on Thunderbird's Lightning calendar plugin: this plugin correctly unescapes the content. Should work for other calendar tools as well.

* Implements esaping of special characters for iCalendar properties of type TEXT. This is defined
* in RFC2445 Section 4.3.11.
*/
static private function escapeICalendarText( $text ) {
Copy link
Member

Choose a reason for hiding this comment

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

static??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure; escaping methods usually are static or global (imagine you would need to create an object to call htmlentities() in PHP). Escaping should not depend on an object instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not going to split hairs here since we have other areas that needs more attention but the static usage would only make sense if used as public where as private it is bound to the object instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. The only point of "static" here is to declare that the method does not depend on object state. A non-static method is a bit like a method that declares a global variable: I would rather avoid it if I don't have a specific use for this additional access. Since "static" (as opposed to global variables) modifies the API, a conceivable future use would also justify non-static methods (you don't want to make a public method non-static later when you change the implementation slightly).

I agree that "public" would be appropriate here too; luckily we probably won't need icalendar escaping anywhere else ;-)

@JeroenDeDauw
Copy link
Member

Generally looks good

@mkroetzsch
Copy link
Contributor Author

By the way, if you want to see it in action, here is a page using it already (with a lot of stuff to escape in the icalendar output):

mwjames added a commit that referenced this pull request Nov 10, 2014
@mwjames mwjames merged commit 1447e77 into master Nov 10, 2014
@mwjames mwjames deleted the issue73 branch November 10, 2014 14:27
@mwjames mwjames added the bugfix label Nov 10, 2014
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

3 participants