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

Fixes and Updates #138

Merged
merged 9 commits into from Dec 4, 2017

Conversation

Projects
None yet
3 participants
@dshanske

dshanske commented Dec 1, 2017

  • Update Emoji Detector and restore 5.3 Compatibility and Testing
  • Add dependencies for PHPCS and Codesniffer Standards to Composer Development Dependencies
  • Fix a Noted Yoda Condition
  • Add a filter to change citation text. Discovered that existing filter would not work as it it used the entire comment text as its variable, not just the citation.
  • Replaced instances of get_comment_meta( 'semantic_linkbacks_type' ) with the get_type function previously introduced.
@pfefferle

This comment has been minimized.

Show comment
Hide comment
@pfefferle

pfefferle Dec 1, 2017

Owner

„All checks have failed“

Owner

pfefferle commented Dec 1, 2017

„All checks have failed“

@dshanske

This comment has been minimized.

Show comment
Hide comment
@dshanske

dshanske Dec 1, 2017

Can you have a look at it more closely? For some reason, the tests you implemented are failing only on the PHP 5.3 install, which has to use an older version of PHPUnit, for example. That isn't something I introduced exactly, it's reenabling PHP5.3 support.

Secondly, the Codeclimate issue is that html5_comment is too long...over 25 lines. Do you really want me to split that?

dshanske commented Dec 1, 2017

Can you have a look at it more closely? For some reason, the tests you implemented are failing only on the PHP 5.3 install, which has to use an older version of PHPUnit, for example. That isn't something I introduced exactly, it's reenabling PHP5.3 support.

Secondly, the Codeclimate issue is that html5_comment is too long...over 25 lines. Do you really want me to split that?

@pfefferle

This comment has been minimized.

Show comment
Hide comment
@pfefferle

pfefferle Dec 1, 2017

Owner

Can you have a look at it more closely? For some reason, the tests you implemented are failing only on the PHP 5.3 install, which has to use an older version of PHPUnit, for example. That isn't something I introduced exactly, it's reenabling PHP5.3 support.

@snarfed introduced the tests and they seem to fail in different situations, for example on emoji tests. Do we really need to be PHP 5.3 compatible?

Secondly, the Codeclimate issue is that html5_comment is too long...over 25 lines. Do you really want me to split that?

no ;)

Owner

pfefferle commented Dec 1, 2017

Can you have a look at it more closely? For some reason, the tests you implemented are failing only on the PHP 5.3 install, which has to use an older version of PHPUnit, for example. That isn't something I introduced exactly, it's reenabling PHP5.3 support.

@snarfed introduced the tests and they seem to fail in different situations, for example on emoji tests. Do we really need to be PHP 5.3 compatible?

Secondly, the Codeclimate issue is that html5_comment is too long...over 25 lines. Do you really want me to split that?

no ;)

@pfefferle

This comment has been minimized.

Show comment
Hide comment
@pfefferle

pfefferle Dec 1, 2017

Owner

@dshanske it seems that you missed to commit the regexp.json from the emoji detector lib

Owner

pfefferle commented Dec 1, 2017

@dshanske it seems that you missed to commit the regexp.json from the emoji detector lib

@dshanske

This comment has been minimized.

Show comment
Hide comment
@dshanske

dshanske Dec 1, 2017

I will try it and try troubleshooting further. Do I really need to refactor html5_comment due length?

dshanske commented Dec 1, 2017

I will try it and try troubleshooting further. Do I really need to refactor html5_comment due length?

@pfefferle

This comment has been minimized.

Show comment
Hide comment
@pfefferle

pfefferle Dec 1, 2017

Owner

Do I really need to refactor html5_comment due length?

No, I already marked it as wontfix

Owner

pfefferle commented Dec 1, 2017

Do I really need to refactor html5_comment due length?

No, I already marked it as wontfix

@dshanske

This comment has been minimized.

Show comment
Hide comment
@dshanske

dshanske Dec 2, 2017

I'm still stumped here. It is only failing on PHP5.3.

dshanske commented Dec 2, 2017

I'm still stumped here. It is only failing on PHP5.3.

@pfefferle

This comment has been minimized.

Show comment
Hide comment
@pfefferle

pfefferle Dec 2, 2017

Owner

at least there is only one last error instead of the three before... are you sure the whole emoji stuff is working under PHP 5.3?

Owner

pfefferle commented Dec 2, 2017

at least there is only one last error instead of the three before... are you sure the whole emoji stuff is working under PHP 5.3?

@dshanske

This comment has been minimized.

Show comment
Hide comment
@dshanske

dshanske Dec 2, 2017

No. That's the idea of testing. I suppose that 5.4 might be the new minimum

dshanske commented Dec 2, 2017

No. That's the idea of testing. I suppose that 5.4 might be the new minimum

@pfefferle

This comment has been minimized.

Show comment
Hide comment
@pfefferle

pfefferle Dec 2, 2017

Owner

I think we should release a new version soon because of some bug fixes, will you give it some more tries, or should we change compatibility? I am ok with 5.4!

Owner

pfefferle commented Dec 2, 2017

I think we should release a new version soon because of some bug fixes, will you give it some more tries, or should we change compatibility? I am ok with 5.4!

@dshanske

This comment has been minimized.

Show comment
Hide comment
@dshanske

dshanske Dec 2, 2017

So am I. If I can't figure it out today, I will revert to 5.4 so you can merge the request

dshanske commented Dec 2, 2017

So am I. If I can't figure it out today, I will revert to 5.4 so you can merge the request

@armingrewe

This comment has been minimized.

Show comment
Hide comment
@armingrewe

armingrewe Dec 2, 2017

Just out of curiosity, why do you want to support 5.3? To my knowledge 5.6 is the only 5 version still receiving (some) support from the PHP crew. At my hoster I believe you have to pay extra to use any PHP5 version (certainly for anything below 5.6, not entirely sure about 5.6), they actively push you towards 7.

armingrewe commented Dec 2, 2017

Just out of curiosity, why do you want to support 5.3? To my knowledge 5.6 is the only 5 version still receiving (some) support from the PHP crew. At my hoster I believe you have to pay extra to use any PHP5 version (certainly for anything below 5.6, not entirely sure about 5.6), they actively push you towards 7.

@dshanske

This comment has been minimized.

Show comment
Hide comment
@dshanske

dshanske Dec 2, 2017

@armingrewe Because people are still on it. The decision we made for the Indieweb plugins was to support the lowest version of PHP that supported what functionality we needed. We've had people unable to use the plugins because they were on 5.3 or 5.4. So, for example, anything with the PHP-MF2 parser needs minimum 5.3.

If you look at Wordpress Stats overall, https://wordpress.org/about/stats/ - 9.8% of people are still on 5.3. So, by making this decision, we just cut off 10% of possible users. But adding functionality beats compatibility in my mind. If we can't reconcile the two, we have to compromise somewhere.

dshanske commented Dec 2, 2017

@armingrewe Because people are still on it. The decision we made for the Indieweb plugins was to support the lowest version of PHP that supported what functionality we needed. We've had people unable to use the plugins because they were on 5.3 or 5.4. So, for example, anything with the PHP-MF2 parser needs minimum 5.3.

If you look at Wordpress Stats overall, https://wordpress.org/about/stats/ - 9.8% of people are still on 5.3. So, by making this decision, we just cut off 10% of possible users. But adding functionality beats compatibility in my mind. If we can't reconcile the two, we have to compromise somewhere.

@dshanske

This comment has been minimized.

Show comment
Hide comment
@dshanske

dshanske Dec 2, 2017

@pfefferle I bumped the version to 3.7.0, added the changelog, and redeclared 5.4 or greater compatibility. If someone installs it on PHP5.3 it should still work, less the emoji stuff, so that should be fine.

dshanske commented Dec 2, 2017

@pfefferle I bumped the version to 3.7.0, added the changelog, and redeclared 5.4 or greater compatibility. If someone installs it on PHP5.3 it should still work, less the emoji stuff, so that should be fine.

@dshanske

This comment has been minimized.

Show comment
Hide comment
@dshanske

dshanske Dec 2, 2017

@armingrewe To give another example, we asked the PHP-MF2 repo and the Emoji detector library to remove short array notation(which didn't come in until 5.4) so the library could be used. But switching from declaring arrays as [] to array() didn't hurt anything.

dshanske commented Dec 2, 2017

@armingrewe To give another example, we asked the PHP-MF2 repo and the Emoji detector library to remove short array notation(which didn't come in until 5.4) so the library could be used. But switching from declaring arrays as [] to array() didn't hurt anything.

@armingrewe

This comment has been minimized.

Show comment
Hide comment
@armingrewe

armingrewe Dec 2, 2017

@dshanske thanks for the explanation, really appreciated. Makes sense now.

armingrewe commented Dec 2, 2017

@dshanske thanks for the explanation, really appreciated. Makes sense now.

@pfefferle

This comment has been minimized.

Show comment
Hide comment
@pfefferle

pfefferle Dec 4, 2017

Owner

thanks, I have to clean up the changelog a bit, because I removed some stuff ;)

Owner

pfefferle commented Dec 4, 2017

thanks, I have to clean up the changelog a bit, because I removed some stuff ;)

@pfefferle pfefferle merged commit 4cda2bc into pfefferle:master Dec 4, 2017

2 checks passed

codeclimate 1 fixed issue
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment