Core Meeting Transcript - September 16th

[wadestriebel]

View in #core on Slack

@norske:

@William_Patton: @James_Nylen

@Joy_Reynolds: hi

@James_Nylen:
!channel core development meeting starting now
So, first thing I wanted to ask/discuss… how many people have a working ClassicPress development environment based on the source code from GitHub, not a release build? Show of :skin-tone-2: s
Alright, a couple of people
The reason I ask is because this is basically the first step for contributing to core effectively
The idea is to have a folder that is (1) a working ClassicPress site and (2) where you can run git commands to manipulate the code

@William_Patton: Do we have a contributor flow document? Or is that something someone is willing to volunteer to write?

@James_Nylen: We have one, but it doesn’t really cover this yet - https://github.com/ClassicPress/ClassicPress/blob/c2c09b27f7dddda5736dc7b9605c437da6554ddf/.github/CONTRIBUTING.md
The way I have my dev environment set up involves lots of Linux and terminal stuff, this is probably not for everyone but there are lots of other ways
To be honest I am not really sure how to explain this in a way that makes sense, precisely because there are so many possible ways to do it

@Joy_Reynolds: Big picture stuff helps. Then the person can work out the details.

@Tim_Kaye: Wouldn’t it be better to explain one way? If we all follow it – at least, those of use who’ve never done it any other way – at least we’re all on the same page.

@Elisabetta_Carrara: Linux girl here… But not so code savvy…

@James_Nylen: “One way” won’t work for everyone, and also knowledge of how the pieces work and fit together is a very good thing

@Tim_Kaye: I understand that knowing why is the key. But sometimes knowing how is the first step to knowing why.

@norske: I usually need 1 example to see key stages of the process. Then I can find tools/ways to perform each part of the process separately

@Elisabetta_Carrara: Not everyone knows what command line is.

@Wade_Striebel: I suppose an alternative is that we do a “simple” way and then everyone who has a unique approach could write up about it

@James_Nylen: I like the suggestion to start with the big picture - what needs to go where and why, and then maybe something like this:
>If you are using OS X or Windows, then we hear that Local by Flywheel helps with a lot of the steps for server setup. You’ll still need to do Y and Z using another tool.

@William_Patton: Getting one single workflow in place will be quite tricky because people like me are set in their ways
What I can suggest though is maybe we make it easy and do ‘group dev’ sessions?

@James_Nylen: Yes, a single workflow is not the way to go. Will not work for those of us who are running Linux.

@William_Patton: We all help each other get setup and then we can pick each others minds for ideas as we test tings.

@Tim_Kaye: I know several of us use Linux Mint as our OS. So we can get Apache working natively. It’s what comes after that that’s the issue.

@James_Nylen: i.e. git and GitHub?

@Tim_Kaye: Yes.

@James_Nylen: which is another big topic

@Tim_Kaye: LOL!

@James_Nylen: I think there again starting with the big picture is valuable, “here are the concepts you need to understand”

@Tim_Kaye: Sure!

@James_Nylen: repositories, branches, how to turn all of that into a viable pull request

@Joy_Reynolds: Who is here to test on Windows? (I’m on Ubuntu also)

@James_Nylen: @Code_Potent works in Windows-land

@William_Patton: the very basics of git are easy to get a hang of and they have served me well for many years (secret: I still only know the basics after those years and am still doing ok!)

@norske: I’m in Win

@James_Nylen: so, there is a lot to cover here, we are not going to solve it all today, but I think it is one of the biggest sticking points for new contributors right now

@Elisabetta_Carrara: Linux. Lubuntu. Eager to learn. Know very basic git. And am not scared of command line

@Wade_Striebel: For that we have the playlist I sent you, we should probably include that somewhere (along with the https://lab.github.com/)

@William_Patton: Contributing to the code is really great but it’s always worth remembering that things like documentation fixes, site updates and sharing news are all valuable contributions as well!

@James_Nylen: oh yes, definitely

@norske: I think we should collect a FAQ somehow. A place to describe personal blockers, then group that into topics, then tutorials

@James_Nylen: so, we already have 3 FAQ pages

@Elisabetta_Carrara: Yeah. But… I kind of understood docs were written in git? Maybe I have totally screwed…

@James_Nylen: let’s have a chat in docs sometime about how to improve all of that

@William_Patton: yes written in git (markdown?) and that means it’s a perfect learning point

@Horlaes: I found out that, the basics of git can be learnt in a day. example, is the link @Wade shared

@James_Nylen: docs are mostly not written in git

@William_Patton: Where are the docs stored then? I always assumed it was a .md type build but are they mostly database entries?

@James_Nylen: that contributing document is a .md file
but we also have docs.classicpress.net and www.classicpress.net/faqs and like 2 other things that I am forgetting right now

@norske: I mean, for example, I faced a problem with upgrading dev release due to a mess with local repositories. This can’t be predicted by more skilled people who write tutorials. It’s easy for them. I guess)

@James_Nylen: so any thinking about FAQs and docs needs to be handled separately, but for now we can start on some resources for local dev, and link it up to https://github.com/ClassicPress/ClassicPress/blob/c2c09b27f7dddda5736dc7b9605c437da6554ddf/.github/CONTRIBUTING.md
to summarize, this should cover basically:

• how do I get a development site set up? (with some pointers to an easy way)
• how does it work, and why does it need to work this way? (this will also tell people how to set more pieces up themselves)
• how do I use this structure to collaborate with others using git and GitHub?
• how do I turn that into a working pull request?
• what do I do if I get stuck on weird errors, have questions, etc?
  I think that is the necessary first step, and we will learn more about FAQs etc by following that process
  anything else in there that you need to know?
  I mean, there’s definitely a lot more, but anything else to get this specific piece unblocked?

@norske: unit testing

@Omukiguy: Tbh: I copy the src into my development environment and work on changes then slap the working changes back to my forked folder then make PR.

@James_Nylen: that works, but you can cut out some steps by just making the src/ folder the same as your development environment

@William_Patton: I’ll just leave this little tidbit here and see how people find they like to use it: git stash

@Joy_Reynolds: my blocker is I’m on a slow, quota bandwidth connection, so I can’t be downloading all these big packages all the time

@James_Nylen: ok, we could keep talking about this but we need to move on
I think that gives us a framework for how to approach the task of teaching/learning more about local development though
sorry, trying to prevent or at least improve a disastrous launch for another client
so, moving on to the current status of ClassicPress 1.1.0
sorry, full tab explosion in progress here…
https://github.com/ClassicPress/ClassicPress/milestone/11
We are in pretty good shape here, I think
@invisnet has been working hard on the new security page
We need to fill out the documentation there, but I think that is mostly just copying/pasting from what is already on the screen itself
Anyone have questions about how this is coming along? @invisnet anything else to add that needs to get done in the next day or two?

@Wade_Striebel: Looks like just translations are left?
For that PR

@James_Nylen: Yep, and there is one section that I think does not fit well because it’s not part of the security page itself

@Wade_Striebel: Cool

@invisnet: mostly done, just need a bit of rephrasing on one bit and that’s it - everything else done locally

@James_Nylen: Ok, cool
I expect we’ll make enhancements and revisions there as we learn more about the needs of plugin developers and users, but this is a good next step and I think it’s pretty clearly explained on the page
So next is the custom image on the login screen, @Omukiguy has been on this for a while and has done a great job staying on top of the “evolving requirements” let’s say
That is https://github.com/ClassicPress/ClassicPress/pull/486 , I think this latest PR has the correct structure with all of the major things we need it to do
If you have a local dev environment and some way to play with these changes and try to break them, please do so

@Wade_Striebel: Is it just the 3 things mentioned on the closed PR left?

@William_Patton: What causes travis to fail on that one?
I see a message about pre-commit checks but it’s not got anything more than that.

@James_Nylen: I think those 3 things are actually already done in the new PR
What does Travis say specifically about the pre-commit checks? There should be a particular file that it’s complaining about

@William_Patton: yeah there are 3 files it complains about. I guess they were modified but I don’t know what precommit checks it needs.
Says to run it with grunt so I guess it’s a thing that was inherited from WP?
Also there are some unit tests as well that seem to have strange fails.

@James_Nylen: loading that page now

@Omukiguy: I fail at unit test so I will require some help there. I guess it’s because we moved a couple of functions from the WP-login.php

@William_Patton: I am not sure if the test fails are actully related to your changes. That is why they are strange to me.

@James_Nylen: yeah, these build failures look pretty nasty

@William_Patton: First thing I see is that emoji library is failing tests on expected versioning.
That’s definitely unrelated to the PR.

@James_Nylen: I think we need to push these commits up again, against the latest version of the code
because this PR originally came from work done back in June, and we have needed to update a good amount of stuff since then
that will be a combination of git commands and running grunt precommit locally until it is happy
so I will take that task
others can help by checking out this version of the code and testing it

@Omukiguy:

@James_Nylen: if you have a local development environment working, then this is something like the following

git remote add bahiirwa [https://github.com/bahiirwa/ClassicPress.git](https://github.com/bahiirwa/ClassicPress.git)
git fetch bahiirwa
git checkout bahiirwa/custom-login-image -B custom-login-image

and try it with the option enabled, disabled … with a site image set, not set… anything you can think of
if you don’t have a local development environment working, but you do have a classicpress site that you can use for this purpose…
https://github.com/bahiirwa/ClassicPress/archive/custom-admin-login-image.zip
grab that zip file, and copy the files that changed into your site, overwriting the existing versions
https://github.com/ClassicPress/ClassicPress/pull/486/files
there is the list of files that changed, or, you can just copy the entire wp-admin and wp-includes directories, like with any other manual install/upgrade/etc

@Omukiguy: wp-login.php also changed from the non-directory files. For manual set up above

@James_Nylen: yes, thank you
that is an important one to copy over

@William_Patton: @James_Nylen if you have phpunit setup there try running WP_Test_REST_Settings_Controller::test_get_items

@James_Nylen: on that branch?

@William_Patton: yes
I think the actual test needs changed
There are a couple more as well that may need adjusted

@James_Nylen: yep, that’s probably a legitimate failure
there is so much noise there that it’s a bit hard to tell right now

@William_Patton: But one thing that concerns me is this tests higlights addition of a new key but it’s at index 0
That means all the other settings keys are incremented by 1 and that MIGHT be an issue.

@James_Nylen: they’re sorted alphabetically

@William_Patton: Equally it MIGHT not be an issue if they are never reference by key index anywhere

@James_Nylen: do we need to expose this setting via the REST API?
this endpoint is accessible for read and write to users who can manage_options, no one else

@William_Patton: Probably needs exposed if it’s ever to be updatable by an external caller.
Like some kind of an app or anything somebody builds headless for CP.

@Omukiguy: I exposed it to XMLRPC. Didn’t do for REST

@James_Nylen: looks like it is exposed for REST
which is fine, we just need to update the test

@William_Patton: rest by default presumably since it’s a core option
When you update that test also add a comment to the docblock saying something like NOTE: key indexes might have been important but we did not preserve them here.
Or something else that makes more sense

@James_Nylen: no, because I know they’re not important
look at lines 96 and 97 in that test file

sort( $expected );
sort( $actual );

@William_Patton: I don’t have the test files opened
guestimating haha

@James_Nylen: that test could definitely be written better, but the sorting and the order is just for the test

@William_Patton: if you can see keys aren’t important then no issue

@James_Nylen:
the error_log is to show me what actually shows up in this REST API endpoint, we won’t commit that

@William_Patton: ok I see - just a static list of known values. Easy fix

@James_Nylen: and the fix is just to add the new option to the list of recognized options

@William_Patton: yep

@James_Nylen: > [“title”,“description”,“cp_login_custom_logo”,“url”,“email”,“timezone”,“date_format”,“time_format”,“start_of_week”,“language”,“use_smilies”,“default_category”,“default_post_format”,“posts_per_page”,“default_ping_status”,“default_comment_status”]

@William_Patton: I wrongly assumed the actual list was dyanmic.

@James_Nylen: those are the settings that come from the REST API
we should probably remove the cp_ prefix from the API because no other setting has anything like that

@William_Patton: OK I gotta head out for a while but I’ll try come back later and see if this still needs some work.

@James_Nylen: ok

@William_Patton: The next thing I was gonna look at was this one:

7) Tests_Rel_No_Follow::test_append_no_follow_with_valueless_attribute
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<p>This is some cool <a href=\"[demo.com](http://demo.com)\" download rel=\"hola nofollow\">Code</a></p>'
+'<p>This is some cool <a href=\"[demo.com](http://demo.com)\" 0=\"download\" rel=\"hola nofollow\">Code</a></p>'

@James_Nylen: kses
I bet rebasing against the current code will fix this

@William_Patton: again I think it’s not related to the PR as such but something else doing silly things XD

@James_Nylen: let’s find outn

@William_Patton: yes I think that is the best idea
The twemoji library being mismatched version as well probably is fixed with a rebase.

@James_Nylen: yep
so here’s what I’m going to run, this will usually fix up any such issues

git fetch upstream
git merge upstream/develop

then re-running that failing test with

phpunit --filter Tests_Rel_No_Follow::test_append_no_follow_with_valueless_attribute

nope, still no good
so I’m going to check out the latest version of the ClassicPress code and then replay @Omukiguy’s commits on top of it

commit 8e6ee7dd585726d0d75391077915407af324fb79
Author: Laurence Bahiirwa 
Date:   Sat Sep 14 15:34:08 2019 +0300

    Add custom login image

that’s the only one we really need right now

@William_Patton: did you try an actual rebase?

@James_Nylen: commit history is too messy

@William_Patton: good call then

@James_Nylen:

git fetch upstream
git checkout upstream/develop -B develop
git checkout -B custom-login-image
git cherry-pick 8e6ee7d

this means: take the latest version of the ClassicPress code from our official repository and apply that commit on top of it

@William_Patton: absolutely unrelated but now you got me wondering what your origin is set as

@James_Nylen: it’s ClassicPress/ClassicPress
but that won’t work for most people so I wrote upstream instead
alright, this commit itself contains some unwanted changes

@norske: James provides an example for us, newbies)
And it’s helpfull

@James_Nylen: there are some problems caused by the approach of copying files over from a site based on a release build
and these files are also not including some of the latest changes from 1.0.2 / WP 4.9.11
trying to think what is the best way to resolve that… I am actually using pretty much all the tools in my toolbox right now
one file at a time I guess
editing src/wp-admin/css/login.css now to change the whitespace back to tabs

git checkout upstream/develop src/wp-admin/css/login.css
git reset
git add -p

that means: put the login.css file back the way it is in our current code, then look at each change and decide whether to accept or reject it
ok, I’ve done that, and pushed the resulting commits up to the same PR
in order to collaborate with multiple people on the same PR, you need to have a pretty solid understanding of repositories, branches, remotes, and how they interact
this is one of the few things that actually works better in Trac, where anyone can upload a patch
but it’s totally not worth it
so, here is how I check my work after processing login.css

the next thing to do is go through all the files in that PR and make sure they were actually things that should be changed
I am not going to do this here, it will take a while
but the general process is to update to the latest version of the base code, apply the needed changes again, look at the changes in the PR and make sure they are correct, and fix/remove anything that shouldn’t be there
so, moving on.
@norske I finally had a chance to look at https://github.com/ClassicPress/ClassicPress/pull/431 a couple of days ago
I think we should pull the <span class="cut"> out of the translated strings and split the translations in two pieces. this will avoid any HTML in the translations which is a plus. We will have to re-translate this string anyway

New version available. <button class="button-link" type="button">Update now</button>

yeah, we’ll have to re-translate everything, no way around that
the other thing we should do is make all the messages consistent, this is partly cleaning up after my unfinished work
so we should end up with __( 'New version available.' ) and __( 'Update now' ) with the appropriate HTML wrapping them
and __( 'Use the "%s" child theme instead!' ) and __( 'This is a parent theme that says "Powered by WordPress" in its footer.' ) also separated, with a <span class="cut"> wrapping the one that should be hidden on mobile
and that structure should be exactly the same on each of the 3 places where this code comes into play
it wasn’t like that when you started this PR, so no worries there
I think we should also tweak the breakpoints a bit
instead of having the .cut hide itself at 1120px, hide it at 780px
this matches the breakpoint where the admin menu disappears
and probably something slightly different in the customizer, since there is a spot around 1000 or 1100px where the theme boxes look very small
hopefully that makes some sense, I know it has been a while since you’ve looked at this

@norske: Ok, I’ll check and finish this in 2 days (hope so)

@James_Nylen: I will probably need to do it before then
release is in 3 days and we need to have a testing build out on wednesday

@norske: ah, I see. I’ll try to find some time tomorrow, in evening

@James_Nylen: if you can
if not, no worries, I didn’t give you a lot of lead-time

@norske: It’s ok

@James_Nylen: we’ll get it done one way or another
and next time with less delays on my part
ok, a couple more things to close out this meeting
I am skipping over a few things in https://github.com/ClassicPress/ClassicPress/milestone/11, they are simpler changes that are pretty much ready and just need testing
https://github.com/ClassicPress/ClassicPress/pull/471 is one
https://github.com/ClassicPress/ClassicPress/issues/476 is a good place for someone to submit a pull request
but please not @invisnet @Omukiguy or @norske, you already have enough on your plate
any other takers and we can quickly talk about what needs to be changed there?

@William_Patton:
I am a bit distracted just now so pick one and tell me which one lol

@James_Nylen:

@William_Patton: Then if you could share any details here I’ll read them when I get a moment.
oh just swapping to the latest jquery. I can PR that tomorrow morning.
What’s the other one?

@James_Nylen: Remind me your GitHub username?
That’s it for now I think

@William_Patton: @pattonwebz is my github

@James_Nylen: Huh, it’s not letting me assign you

@William_Patton: I am not in your team
I think I need to comment first, will do that now

@James_Nylen: done
I think just what it says in the issue, and test it on a site and make sure that doesn’t explode everything

@William_Patton: yeah looks easy enough, more time testing than the change

@James_Nylen: right

@William_Patton: Give me a chance to spin up a couple CP test beds though which will be handy for other things too

@James_Nylen: yep, I did the same this weekend (relatively fresh OS install)
so I think I will just drop this git tip here to end the meeting (finally)

git fetch upstream
git checkout upstream/develop -B develop
git checkout update/your-branch-name
# make your changes
git add src/your-changed-file.php
git commit # type the commit message in your editor
git push origin update/your-branch-name

@Omukiguy: Will this meeting notes be archived? So much to learn in Git here

@James_Nylen: that is a good sequence of commands to use every time you need to start a new pull request. this will give you a clean history without the problems we’ve been seeing above.
yes, it will be a long thread but we can do a forum post. I expect we will also spend a lot of time unpacking this into guidelines and tutorials which is fine.
structure & organization is different between a real-time meeting and a reference document, but we’ll figure it out.
so, that was a lot of stuff, but hopefully it was a good glimpse of what needs to be done in order to wrangle all of this into a shippable release.
that’s all I wanted to cover today I think. open floor for questions, complaints, etc.

@William_Patton: ohhh if you are asking me for complaints I have lots.
But if they need to be about this here meeting then I’ve got none
but complaints about other things!!! ohhhh sooo many haha

@James_Nylen: inb4 “git is byzantine”

@norske: Thanks for the meeting and for the mentorship! Good night)

@James_Nylen: goodnight
and you’re welcome
it is definitely more natural for me to “do” than to “teach”, but maybe that is what I am here to learn right now
I could say more but I will take a break instead. Thanks everyone for your time.

@William_Patton: you deserve that break by why do I have the feeling a break from here means you needing to return to that problem with the clients site you were dealing with earlier ?