Install drush with composer #714
Conversation
Install drush with composer Split out phpcs Try to get 5.3.3 working Clean up symlinks Use older drush for 5.3.3 Fix script
callable is really a string Fix tests
There was a problem hiding this comment.
Ok, this sounds weird. The code is good, but there are implications here we need to deal as a community. Can we ask TAG to go through the functions that got their signatures bumped to actual type casted arguments and make sure calls match from other modules? A spreadsheet with links to its use in the code base could do wonders (grep-ing). I added some comments in the code. I feel we can not merge this until we decide how much of the code (like every islandora module) are we going to touch?
| @@ -152,7 +152,7 @@ class DublinCore { | |||
| * @return DublinCore | |||
| * The instantiated object. | |||
| */ | |||
| public static function importFromXMLString($dc_xml) { | |||
| public static function importFromXmlString($dc_xml) { | |||
There was a problem hiding this comment.
This change affects (nominally because php is case insensitive for functions/methods http://fr.php.net/manual/en/functions.user-defined.php). I tracked this use to almost every SP that we have. (webarchive, large image, pdf, you name it)
| @@ -899,7 +902,7 @@ function islandora_ingest_form_get_object(array &$form_state) { | |||
| * | |||
| * @param array $form_state | |||
| * The Drupal form state. | |||
| * @param array $step_id | |||
| * @param string $step_id | |||
|
I'm wondering what the implications of using 2 different code sniffer versions are here? Is it possible we will get into a case where one code sniffer says one thing, and the other code sniffer says something else or is it all using the same sniffs? |
|
@DiegoPino am I correct that you would like TAG to go through all the code and look for any occurrence of the methods changed here? That seems a little outside the mandate of TAG, though I guess we can throw the decision on whether we should do that to them. But I think the work will need to be distributed. Also as this will probably have the effect of revealing alot of new coder violations each module will need some clean up. |
|
@whikloj i like TAG to do whatever TAG is in charge of (organizing this, saying yes it needs to be done or even finding a workflow that allows us to keep getting contributions) . That way it is a community/foundation effort and we do have agreement. TAG is the only instance out of the committers calls that have that attribution/tech role. I was (i hope so) not implying you all doing the work. As you correctly stated this will reveal a lot of mess in our code, specially in our use of PHPdocs and variable naming and to be honest it needs to happen all in a pretty close period of time. And the function argument thing could even expose bugs(fatalist). Could be a release task for the upcoming one or a sprint, i just know this can't come from the same 5 suspects (you are one of them) that normally do all the heavy work (and we all are grateful). Also, could someone add a @islandora/tag group so we can talk to TAG as an individual here? Merci |
|
@jonathangreen That is a good question, I had to go with the different version of coder as PHP 5.3.3 needed an older version of PHP_codesniffer which couldn't support the (IDK "format" 🤷♂️ ) of the newer sniffs. I did have one situation where this appeared here where one wanted a space around the unary operator and the other...wanted something else. So I modified the code as I found this more readable anyways. |
|
@DiegoPino I'm not a fan of that function rename either (case insensitivity in function names seems like a terrible idea in the first place), but this will sort itself out over time as we push up compliant code. By next release it's a given that I'm more worried about @jonathangreen's concern, because I'm sure drift is inevitable. But if we want to support 5.3 then we gotta live with the two versions. Just another tally mark against 5.3 at this stage in the game. But are we ok to move ahead with this one? 7.x is pretty much at a standstill until this gets resolved 😢 |
DiegoPino
dismissed
their stale review
Tech lead says we will deal with lower/upper case and code sniffer interpretation of the drupal coding standards on a pull by pull basis. So my concerns are gone. I don't feel super prepared to do a full review here, could i get a second pair of eyes?
|
@whikloj @jonathangreen @dannylamb ok, i dismissed my review. Seems like there is consensus on fixing this in every repo as we go?.I did not see any TAG interaction so i will assume this is it. Can do a full code review tomorrow but if someone has some better skilled eyes and can do it sooner feel free to open the gates and give it a green light. Thanks! |
|
So we don't have old versions of PHPCS sniffs conflicting with new versions, I would like to propose that we don't bother with PHPCS on PHP 5.3. We can still run the tests, just not run code sniffer. Thanks for all the hard work on this @whikloj. |
|
I agree with Jon, no sniffer for 5.3
El El mar, 29 de ene. de 2019 a las 21:35, Jonathan Green <
notifications@github.com> escribió:
So we don't have old versions of PHPCS sniffs conflicting with new
versions, I would like to propose that we don't bother with PHPCS on PHP
5.3. We can still run the tests, just not run code sniffer.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#714 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGn854_X5dSJCG2bp9nQ4A7FsqnmcSdfks5vIQT6gaJpZM4aT7k7>
.
--
Diego Pino Navarro
Digital Repositories Developer
Metropolitan New York Library Council (METRO)
|
|
I concur. It's better in the long run to just jettison the 5.3 sniffs now. |
|
Ok just to be clear we don't want to run |
|
Also should I make PHP 5.3.3 try to install the latest version of coder too? |
|
What is coder used for (in our setup) other than providing the DCS? If just that and we are not doing sniffing then maybe not needed? |
|
@DiegoPino it does these checks, previously it also ran the code sniffs. |
|
@whikloj @jonathangreen: I suspect there may be issues with the object access callbacks as well; for example:
Could end up with PHP errors popping up when attempting to access non-existent objects? Not sure of actual behaviour, but HTTP 500 kind of things instead of the expected 400s, possibly? |
|
I tested on my site going to a non-existent object with this code, and I do properly receive a 401. I also tested putting a policy on an object to restrict it to a user, then accessing it as anonymous, in that case I get: |
|
That seems to be a bug or this existing documentation for that function was disregarded at some point. |
|
Digging deep here, its been a long while, but we use |
|
So I was not getting the same FITS problem in my test vagrant. I tried to remove the TECHMD datastream and still the object seemed okay. I must have been missing something. So I removed the hint and @param documentation from the two previously discussed functions. If you want to try it again. We do need to maintain some standards as this is an OSS community, but I'm not sure how much we can expect. Maybe this is good enough and we worry about doing better in Islandora 8? |
|
Rather than delete the documentation (as I did in this commit) I'm trying to just expand it, so I re-added the docs and (hopefully) corrected them. Let me know if you hit any others, we don't embargo anything so that is one I never think of. |
There was a problem hiding this comment.
This is a big commit to core, and its changes (especially the type hints) may still break things, in custom modules or other modules. However I think its important to keep it moving along, as it also is important to get travis fixed in our codebase, and it doesn't seem like pear is coming back anytime soon. I also think its great work on @whikloj part to take on fixing travis and modernize the code-sniffer tool chain we are using.
This addresses my concerns and works on my test site, so I'm going to approve it and start the 24 hour clock 🕐 for merging. Since it's a Friday, I'd like to wait until Tuesday to merge, assuming there are no more comments. If any @Islandora/7-x-1-x-committers are interested, I would encourage them to take a look and give it a test in their environment.
|
|
|
@whikloj @jonathangreen |
|
@DonRichards if you want to submit a fix PR with testing instructions I will review and merge. |
|
Created a ticket. I'll look at what changed and see if I can identify the point failure (missing var in function call). https://jira.duraspace.org/browse/ISLANDORA-2387 |
|
And just like that, it started working. Not sure what changed. I assume my vm didn't pull in the changes from other solution packs that were related. Looks fine now. Sorry for that. |
JIRA Ticket: n/a
Discussed at the January 24, 2019 committer's call.
What does this Pull Request do?
Due to PEAR issues, this PR installs drush using composer. It also installs coder and PHP Codesniffer this way.
For PHP 5.3.3 it installs:
For all newer PHP version it installs:
Finally it updates the
travis_scripts.shto ensure appropriate response in the case that the final test (phpcpd) passes but previous have failed.What's new?
There are 2 commits here.
The first commit contains the setup changes.
The second commit contains all the code updates to account for the new coder sniffs.
How should this be tested?
Travis should pass.
Additional Notes:
There are two things of concern in the DublinCore class.
d0d9d16#diff-e8e5de937161791c32db421e1faec58c
These were required by the sniffer, but I am wondering if they will cause failure to others using these functions?
I'm not sure if PHP is case sensitive for function names, but my IDE warned my about adding in the old function names to pass along with deprecation notices.
If they are case-sensitive, perhaps nothing needs to be done. If they aren't case-sensitive we could ignore these functions for the code sniffer. 🤷♂️
Interested parties
@Islandora/7-x-1-x-committers specifically @dannylamb, @jonathangreen and @adam-vessey