 Welcome back to Drupal Core Therapy. Sorry, I'm in the core conversation track. I'm XJM, that's on Drupal.org. My name is Jess. I'm probably best known in the Drupal community for the core contribution mentoring program. But I'm also one of four co-leads for the views in Drupal Core Initiative. And more recently, I work in the office of the CTO at Aquia and my job title is Code and Community Strategist. So what that means is that Dries is my boss and I work on core stuff basically full-time, which is pretty cool. So the title of this core conversation is Making Big Changes. And that's big changes in Drupal Core, not in your personal life, sorry. I'm going to really describe four different case studies of big patches in Drupal Core and then summarize some of the lessons I think we learned from them. And then after that we can sort of discuss what to do about it. But first I'd like to scope this a little bit. I'm going to leave aside for now anything about how to run an initiative in general. I think Shannon already covered that. And I also want to talk more specifically about individual issues, individual patches in core and also issues that are outside the scope of any one initiative. I also don't want to talk about tools we don't have yet, like GitHub, like improvements to the Drupal.org issue queues because I think that there's been at least one session and one core conversation devoted to those topics already. And finally, I don't want to talk about whether our workflow works best with patches versus sandboxes versus core branches versus unicorns because I don't really think that we'd resolve that or even... I think if we started that debate it would actually be in here until code freeze. So I'm going to skip past that for now at least in my portion of this conversation. Calling it a conversation is a little bit strange because I talk at you for half of it and then there's the conversing part. So it needs a different label. So we have the saying in Drupal Core that the drop is always moving. And this means that we're willing to innovate and make bold changes that make Drupal better. At the same time though, big changes expose us to risks and they can be difficult to manage. They add to our technical debt and even if our goal is to reduce the technical debt in the long term, in the short term they're definitely inducing more and that's a risk. They lead to issues with hundreds of comments. This frustrates all of us and it leads to people burning out and tuning out. Big patches are... they break a lot. They're very difficult to re-roll and if the need comes they're also very difficult to roll back. And a particular problem that's close to my heart they're freaking hard to review. And for the algorithms above they cause contributed burnout. Whether you're reviewing the patch, whether you're testing the patch, whether you're just someone following the patch who's interested in it and especially through the patch author. It can be really discouraging. Even if you're successful in the end it can be a very trying experience. I'm going to describe four examples of big changes in core during the Drupalate release cycle and just walk through roughly what happened in those issues. And the first change in the issue is probably my favorite moment in the entire Drupalate release cycle when views was merged into core. And I'm going to just describe a little bit how the VDC worked. So we started out with an 8x-3x branch of views in the views project repository and gradually worked on removing views external dependencies mostly on C tools. And stuff that we thought would be useful in the rest of core, like drop buttons, the temporary storage, temp store, config entities, ended up on their own as issues in the normal core queue. So those issues went through the normal core process and were adapted to core separately before we went to merge views into core. But once those dependencies were resolved then Tim built us a sandbox that had views as a core module. And Jeremy Thorson set us up so that TestBot could test that, which was invaluable because core API changes were breaking our sandbox pretty much on a daily basis. And so it was really important to have that feedback to know whether or not it broke again. And we knew going in that we had like seven years of technical debt to resolve. So once that we had the sandbox I also spent some time crafting what I helped was like a bullet-proof issue summary that goes back to what you were talking about in your previous core conversation. And Jeremy like wrote it in a Google doc and reviewed it with the whole views in core team before I even posted it because I really wanted to make sure that the scope of the issue was clearly defined. We had done a lot of cleanup and refactoring but at the same time, you know, it's like the patch that we rolled to test views against core was 2.5 megabytes and views has like a change that involved 500 files. So we ended up merging views in on October 22nd in 2012 and that means that views has been in Cornell for exactly seven months. Seven months, guys. I should point out that we did make some exceptions to the normal core process when we merged views in. There are still core gates issues open for views. In our issue summary, we listed out the things that were not part of the issue like coding standards cleanup. So we also identified some things that if views had been a normal just core patch should have been part of it. We knew that there were accessibility problems in views that we weren't going to be able to fix in part because we were blocked on some issues in core. And as a matter of fact, we still have several accessibility issues, known issues that have been open for a long time because we're blocked on this one particular issue. So if you have an interest in helping with the modal dialogues to make it so that views is usable with a keyboard, that would be amazing and come to the Code Sprint on Friday I didn't even look up. And there are also usability issues. We did a usability study on views at bad camp and I say we like I planned it all I did was sit there and go oh my goodness but some of the usability contributors in the Drupal community set up a usability study and did that. There were also performance concerns going in. We knew that our test coverage wasn't perfect but at the same time it was probably the right decision to make to merge views in at that point in the release cycle to be able to finish all of those things by the December 1st Code Freeze. And it did a lot of good for the rest of CORE for views to be in that early because we were really able to have use cases to test some APIs that wouldn't have been available without a module with like views there. The second change I'd like to talk about is the Blocks' Plugins conversion. And this is the one that led to a big retrospective with all of the initiative owners and then in turn to this CORE conversation being proposed. So if it seems like I'm picking on this issue that's the reason for it and it was a pretty important learning experience for all of us. So in April of 2012, Chris Van Der Water Eclipse GC posted a 144k patch that added a new plugin API a new block system and a conditions module and it also converted every single block in CORE to this new API and it also converted the configured block instances to the then HTML based configuration management system. For those of you who are familiar with CMI that's how long ago we're talking about here. So once the plugins system went in separately in August we just done from April to August Chris posted an updated patch and then he also added a UI overhaul into the mix to make the new user interface a new user interface to support a new model for placing blocks. So while the patch converted all of the blocks it didn't actually at that point update any of the test coverage in fact the issue was marked active until August until catch came and said it needs review and was like what's going on here. And of course you know there were like hundreds of test failures because all these tests were using a block system that wasn't there anymore. So a small group of dedicated superheroes, Nasho, Camilla, Anderson I think is our last name and several developers from ZivTech spent a bunch of time fixing those tests just to convert them to the new API. The entire sprints dedicated to just getting the old tests to pass for this one patch whole sprints. And by the time the patch was passing it was 400K. So that's when I came into the issue and it took me 80 hours to read and review the entire patch and I spent the better part of a transatlantic flight writing of a comprehensive 3,000 word summary of my other reviews on this issue. I documented the remaining tasks file follow-ups for it. I thought this issue was really important. You know it was doing something that was laid in our architectural foundation for actually views in the future as well. I tested it manually. I helped with some minor architectural issues and then I invested another 30 hours of work or so in that part of it. And meanwhile merging head for this particular patch because it was so big it was still failing on other core API changes daily so Tim Plunk and I spent a lot of time just fixing those merge conflicts over and over and over again. So on December 20th, now we're from April to December 20th, we had this big call with 3s and several people from the blocks and layouts initiative and we came to the conclusion that the only way the only way the Scottish initiative was going to make any progress in this release cycle was to commit this patch now and just do the rest as follow-ups because there was so much overhead in maintaining it. So the idea was that after that happened we would start crowdsourcing this follow-up issues because it was some things were no longer condensed in this one 300 comment, 440K patch we would be able to get other people to make just one specific change and it would be easier to find people to do that. And so we committed the patch on January 4th with over 50 follow-ups already filed. I filed most of those myself even though I'm not part of the blocks and layouts initiative and we bent the rules pretty dramatically for that patch. It did not provide an upgrade path for blocks at all. It introduced a 15% performance regression at the one particular case that we profiled. It had numerous UI usability issues and the test coverage and documentation were also incomplete. Now those issues still aren't resolved. So what we have is fairly significant architectural debt in core right now. We made a significant improvement but at the same time the trade-off has been a persistent risk to the release of Drupal 8. Now I want to pause for a second here and talk about the similarities and differences between views in core and the blocks as plugins patch. Now note that both patches added new APIs to core and both had some exceptions granted to our normal policies in order to save them from this re-roll hell that we were stuck in where we couldn't make progress because we were spending so much time chasing head. Both had concerns about usability and accessibility and performance and test coverage and both spawned a lot of follow-up issues and issues that still persist in all these areas in some cases. So that's important to note. It's not that views was magic and wonderful and blocks did a horrible thing. There's definitely some similarities there. But there's also some key differences that have sort of effect how this is played out over the course of the release cycle and the first thing is that the views API which it's not perfect, it has been battle tested for seven years as a contributed module. It's installed on 70% of Drupal sites, so there's definitely issues with it. But we have that community testing over time. Whereas blocks was introducing an entirely new API, the plugin system based conceptually on C tools but not the same thing. And also, and this is important, views went in three months before the first feature freeze. We're talking about the difference between October 22nd and January 4th. And that's a big gap in terms of the release cycle, especially the last year of the Drupal release cycle. Also, views unlike blocks and plugins didn't actually significantly impact the rest of core when it went in. In fact, if we had done the equivalent of what the blocks as plugins patch had tried to do in views, every core listing would have had to have been converted to a view before we merged views in and we've done one of them so far by now in mid-May. We're working on three more that should land I think soon. You put your head down. Did it go in? No. Okay. See, I thought you were putting your head down because I didn't touch the cue. So they will go I have faith that there are at least one of these issues will go on this Friday. I'll put it that way. But yeah, so if we had tried to convert every listing in core to two views before we merged views, they just wouldn't have been possible. And finally, the community made it a focus to start addressing views, gates issues right away. Like I said, we had a usability study done at bad camp which was just like a week later and the accessibility issues, we tried to start working on them right away and then got blocked on this one thing that we couldn't really call it sourced. It's a problem that I don't know how to solve. So it's there are similar situations but there's also some important differences. I'd like to move on to our third change. This is the field API to CMI conversion. Swentell filed this patch last August and to start it was only 37K and they got the patch passing test pretty early in it and people started reviewing it right away. The issue summary was clear, succinct. It was regularly updated with the changes and status of the issue, issues related to it. They worked on the conversion in a sandbox and they started adding an upgrade path in October. Now, the original issue had been posted before DrupalCon Munich which is when we added the configuration entity API decor and after we added that API it was pretty clear that fields should also use it for configured field instances. So they had to convert to that as well and Alex started working on that in November. I can hear. Okay, so the thing that was most amazing about this issue is they always responded right away when someone posted something. Even when there was, over the winter there was sort of a period of silence when they were kind of blocked on an upgrade path problem they still responded right away when someone posted the issue, what's going on here? So that was cool. So Justin Bejibus did some performance testing on this in the spring and they also worked with him they posted sort of the canonical patch for it, the one that they said they thought was ready to be reviewed in March, right? March 15th I think and it was passing tests and they actually updated the issue summary to indicate that they thought the issue was ready for serious reviews which was awesome. So they, we myself and Alex Pott and Alex Bronsied did some detailed reviews and they, each time we posted a review they followed up responding to every point that we discussed and it actually, so the patch was 250K in the end but it only in quotes took me about eight hours to review it as opposed to the 80 I spent as the Blocks's plugins patch which was less than twice as large so there's a significant difference there and I'll get into in my lessons, learn why I think that is so they, you know, we had some miscommunication misunderstandings originally but they did, they were pretty supportive in terms of like filing follow-up issues right away to make sure that things that I brought up that were out of scope for what they were trying to do ended up in follow-up issues by the first week in April they were still working through our feedback, you know, because it was a big patch there was, and I read patches line by line so I posted quite a lot of information and why should I express that it was difficult to address all the feedback that we posted for such a significant change for such a large patch and the words that he used were actually that it was daunting and somewhat soul-crushing so after that I felt really bad and as soon as I could make time I tried to help like filter out some of the noise in my reviews and other reviews I swent all gave me access to their sandbox and I made some just really stupid minor cleanups that I'd identified in my reviews did the best I could do with their documentation as far as I could understand it and then condensed my review down into one post that listed everything and that actually worked out pretty well because I had the privilege of RTB seeing it four days later and then it was committed live at what front-end United, just the name of it, yeah and there's a video you can even watch them live when the patch is committed, it's pretty cool and I have to say that like reviewing that patch after the experiences with blocks and plugins was rewarding because it reminded me that as a community of people working under Drupal Core we should really trust each other and I wish that I'd remembered that going in and the fourth big change is one that's still under way and that's the twig conversion the converting PHP templates to twig templates it's actually only a part of what the twig unofficial initiative is trying to do and I should give a little background here there have been issues to convert templates to twig which is hopefully going to be our new theming system in Drupal open since November but they for a while they sort of trickled along and then this spring a bunch of people started working on them and if you work on core issues you might have noticed recently that a bunch of them were sitting at RTBC which stands for reviewed and tested by the community I forgot to say that earlier it means that the issue is ready for a core maintainer to look at it it's gone through our QA process but since it's so late in the release cycle it's already after our feature freeze and approaching our code freeze Dries has been working with the other core maintainers and with the leads of a couple initiatives to try to mitigate the risk that making changes at this point is going to push back our release date because that's something we definitely don't want we saw the negative impact of that in Drupal 7 and as much as possible we want to get Drupal 8 out the door sooner rather than later so at this point in the release cycle we don't want to have technical debt so what the twig team agreed to do is that so that we don't have core half in one templating system and half in another templating system which would be completely unusable for themers anywhere and make their lives much worse instead of better we're going to convert all of the templates in one patch and a lot of the individual templates are done already in individual issues but what they're going to do is sort of the equivalent of pull requests on GitHub but at the same time they're using the issue queue patchwork flow to do it both because that's what they were most comfortable with and also because it gave them the visibility of the core queue to get this stuff reviewed so that's still underway and it's making great progress but it definitely followed a very different pattern from these other massive patches so I have a bunch of lessons list here and this is the point at which my presentation sort of collapses into just what I think I was hoping to sort of consolidate this a little bit into more important messages but I'm just going to go through them because I didn't really have time I really overbooked myself this conference it turns out that you can't run have eight or ten technical discussions try to run a sprint and present it three or four different things that's just a really bad idea and don't ever do it and I'm not going to again and also these lessons are things that I think were important they come out of my experience as a reviewer and more recently my experience working directly with Driesen Webchik as core maintainers so lesson number one is that the risk of a big change and whether that risk is a concern depends on when we are in the release cycle and this is something we really didn't think about with Blox's plugins as much as we should have so merging views in October was different from adding Blox's plugins in January and that was different from trying to get Twig in now in May or early June and that's something we'll need to keep in mind as we go forward and well in this release cycle it's already a focus and Driesen is very concerned about it but it's also something we'll need to keep in mind I think through the whole initiative in triple nine or sorry the whole release cycle in triple nine second lesson, lesson in three parts and if you remember nothing else from my whole talk I should remember this which also means that I should have put it at the beginning or the end instead of in the middle this is me not thinking ahead but clearly define the scope of this issue what I mean is say in your issue what's involved, what you're going to do and what you're not going to do and don't mix apple and oranges only make one kind of change in an issue as much as possible and also document that in the summary of the issue so that other people looking at it don't say oh hey XJM I would really like to help with the coding standard cleanups in your 2.5 megabyte views patch no we're not doing that now please come and see me later in May when we have the sprint on that for example triple con portland and so say what you're going to do say what you're not going to do and part of that is this is something we actually had a lot of success with in views in the process of working on the main API for something you find something else that you need from it first whether it's another new API or just refactoring some automated test to be more performant and more decoupled just immediately split that off and do it in a separate issue in the cork because you'll be able to get that in that will make your patch smaller and reduce the burden on this one particular point of failure for what the change you're trying to make plus it also benefits the rest of the core at the same time and ensures that one piece of it isn't going to break the next time someone else makes an update to that file and part 2c is file follow up issues proactively so someone comes to you and says okay I tested this with taxonomy and that when I deploy a configuration change with a taxonomy field it breaks because it's referencing the taxonomy term ID instead of the UUID and that's broken point out that there's already another issue addressing that problem it's because of the fact that taxonomy terms entity reference fields and taxonomy reference fields are not yet using unique identifiers I probably shouldn't have gone through that whole example but any kind of thing that is totally out of the scope of your initiative and you're afraid we'll derail the discussion in your initiative you can just immediately fire back with another issue when we merge views and we do this very aggressively because it's a relevant discussion not part of our discussion here and that will make sure any comments about that shunt off to that issue you don't have to deal with it until later and again the difference between what's a blocker and what's a follow up is important I have two parts there they're similar but they're different because one set of things you need to do beforehand and one set of things you're going to do after and the set of things you do after can't include regressions if your patch introduces a regression you always need to fix it in that patch it shouldn't include things that are the core gates usability accessibility testing documentation and performance unless you're actually like you've communicated with a core maintainer and gotten approval that that regression or that gap is acceptable for a short term part three please update your issue summaries please keep them up to date start with one that's useful so people look at it and keep adding information to it to communicate to other people who want to work on it what the status of the issue is it's amazing I so I I mentioned that I have the opportunity to look see somewhat how how Dries and WebJig review patches and it's it the number of times that Dries stops and says okay so so what is the goal of making this change and it's like step back why are we really doing this and that's a very important question to ask so I think that part of our job as as developers is to communicate what what are actually our goals are that we're not just refactoring for the sake of refactoring there's an end there's an end goal that we're working toward um add your test coverage in docs thoroughly I use I use automated test as a way to explain to me what a piece of code is expected to do and so when when a patch does not provide new unit ish test coverage for its new apis that's very it makes it more difficult for me to review that's one of the challenges I encounter with the blocks is plugins patch the other challenge was that it it wasn't very thoroughly documented and documentation by the way doesn't mean just adding a stub one liner that follows our rather anal documentation standards for a function or a class or whatever it also means actually explaining what your code does um so that means like paragraphs that people can read that explain how your architecture fits together and what the big picture is provide in our diffs um that's all I have to say about that um this is a trick we used um if you're trying to get test to pass uh do that first it's a helper issue to just reduce the noise and the main issue it means that there's less there's fewer times that people have to look and find out that oh your test failed again and it makes it easier for people to scan down on what's seeing see what's going on smaller patches are more successful um if you're making a big change that's kind of a problem but think about how you can make your patch smaller um and in terms of reviewing the patch in terms of re-rolling it in terms of someone understanding it I think I do okay with about 50 to 60k of patches in one sitting obviously it depends on the kind of change but I mean 50 to 60k of actual code changes when it gets to be about 100k I start to burn out and when it's over that I personally like have to break it up I can't even go through it in one sitting um that said your meta conversion issues to do the follow-ups will take a lot longer than you expect um we've seen this in views we've seen this in the configuration management system if you're planning to introduce an API first and then change things to it later just keep in mind that you if you think you're gonna be done by 2012 you'll be done in 2013 also another thing that can be troublesome about this is that sometimes it's hard to understand the big picture from just one individual patch um so that's something that to keep in mind I think that that can be best addressed through by having having a roadmap and documenting clearly what your overall goal is like I mentioned earlier start with the test implementation so just convert one thing that shows your thing works in an automated test and that will explain your API and it will also keep you from you know you've verified that it works without you having to convert everything um and use a backwards compatibility layer um this is something that we've argued a lot about in this release cycle whether or not we should temporarily and uh a backwards compatibility layer so that we can use code that's using the old API while we're interested in the new API and overall I think we've come down on the side of yes for the during the development phase especially it's it's a necessary part of getting change done um the closer that we get to release it becomes more problematic and obviously there's if if we're not going to be able to remove that backwards compatibility layer that's a problem but overall it's been successful also number eight is help help people who are reviewing your patches um there's just not enough people in core who are reviewing changes it's it's time consuming it's a challenging skill to learn um reviewers are looking at a lot of different issues in the day so so you know be nice to them help them if if help them find where in your issue it says a certain thing that you know that they don't um help them file follow-up issues and also indicate in your issue what you want reviewed um you know it could be that you're at a point where you want architectural review or you're just looking to have review of one piece tell us that we we if we know that then we can focus our attention and not waste time on things that aren't ready yet um and and also please keep in mind that that people who are volunteer contributors who are not trying to you know be gatekeepers and block your work they're also trying to help your patch get in so converse side of that is help people who's code you are reviewing um and this is something you know this is this is the lesson that I forgot when I was looking at the field CMI patch need to stop doing that um you know make make your reviews organized summarize your own points don't just sit there and go you need white space error here in this comments and then you can get a little bit of substance to feedback because that's a waste of everyone's time make those changes yourself or find a new contributor who will actually get benefit out of making those fixes in that patch um we want people to be validated by our Q&A process and not feel like um we're we're attacking them basically um be an issue manager when you're making a change don't just think about the patch think about controlling the entire discussion in the issue um it's discussed um it's your job to be like a PR person for your change to communicate with people about it um and to keep all the information the issue together because like an issue if if all we needed was the patch we wouldn't have issues we would just have patches we need to actually communicate about the changes that we're making otherwise what's the point um and number 11 this is something Shannon talked about is build a team this doesn't have to be something on the scale that the you know the views team um it's just really helpful to have at least one other person who's also working on the issue with you because they can give you meaningful feedback um they know what you're going through you can trade reviews with them um do back and forth work on it and also if you end up getting burned out on the issue there's someone to take it over for you for a period of time um we this for example the admin people issue where we're converting the admin people listening to a view and it's gotten stuck a number of times on both weird bugs and uh maintain our feedback and so we've I think we've gone through we've gone through the entire team at this point um but it's it it can help a lot just that when you're when you're tired of working on something have someone else to step up and take over so make your issue readable make it reviewable make it understandable and help each other out and also cut down on the noise that's in the issue those are my thoughts um and so yeah let's talk about it I have I have this slide that says I prefer not to talk about sandboxes and githubs and issue q improvements and all that I'm more interested in your thoughts on like actually you know making changes and whether you think that things that I've said are a big that lie um but we can also talk about that if people get bored if you file up good if you file good follow-up issues and organize lots of meta issues you might recruit the next core maintainer yes by giving things uh by by creating um follow-up issues matter issues like all the CMI issues that Greg created got me involved in he recruited the next core maintainer so and you also teach people to use your system so if you do everything at once then no one knows apart from you like we've got a show and the poor sop who reviewed the patch yeah so I wanted to expand on that and another point from you I wanted to expand on that and another point from you earlier um as the risk management and and how to do your stuff so so my tip there is do stuff early very early so if you do stuff early there's a mountain of advantages you use the API from the previous version of Drupal because there's not much changed so it's easy to do the change that you want to do yeah we converted views in what was it three hours Tim right two hours so if you do stuff early you don't need to convert like to all the new systems that were introduced if you do stuff early then the responsibility of converting them to the system will not necessarily be on you because that will be in the system so others will need to work with it if you do stuff early you will be able to to to advertise your change and others will be able to work with it so whatever you did will be an integral part of the system and not feel like a parallel thing that you did because you weren't working with the other things that were already there so you will be able to get feedback and refine your work so like Greg re-architected CMI three times because he did the initial iteration first instead of like just working on the issue himself and getting it into core and getting that visibility so there's just a mountain of advantages there doing it early I think we've had some of our issues like this where people said no no you shouldn't do this because there's this re-architecting going on maybe later it will be this new subsystem that will make it totally different and we're like no we're doing this right now and it paid off very well we have all those features and the other thing is the communication thing so I think documenting what you do very well is very important first it makes people aware of what you do and sometimes we end up with parallel systems that are very similar but they are different because people have been working on them parallely and they were not aware of the other solutions and if we document what we do and why we do it then we can get people learn them so what I did for example with the configuration schema system is actually implementing configuration schema is not that hard there are complex examples but the basics are easy so when it was started I wrote like a huge documentation page with examples and then I opened a whole lot of issues and then I expected and I was trying to recruit people to work on them and expected I can crowdsource it and not just crowdsource it so it's ready sooner but crowdsource it so people validate my documentation and they validate the schema system in itself and put feedback into improving it so if I do it early I have time to those breakout issues I have time to have based on documentation get people improve my system because they work on it and can contribute back what happened there is unfortunately unfortunately and also fortunately I have a person who solved all of the issues so that was that was good because now we have all of the schemas there it was not good because knowledge was not spread but I think if you have that documentation been down that helps a lot there I would agree so something I'm actually wondering about is how many people in this room are familiar with what the Drupal 8 release schedule looks like like have an idea of when Drupal 8 is being released or wow okay okay have an idea when code freezes that's probably a safer question to ask okay how many people are familiar with the what the core gates are Drupal core gates wow okay yeah okay so I'm not going to actually get a very small number of people raise their hands and answer the second question so that's something I mentioned in my discussion about because we have our review process for Drupal core is very rigorous and we don't ever want to address our functionality in these five key areas that I mentioned several times in my talk but we don't want to make Drupal slower we manage to make Drupal a lot slower during Drupal 8 like significantly so we need to work on that one we always make sure that we add test coverage for changes that we make so that if there's a bug we don't break it a second time for the third gates accessibility we don't ever want to regress our accessibility coverage and views did because there's all of a sudden there's this very useful important module in core that's not usable at all with a keyboard because we can't we haven't converted views from its previous broken models to use the new accessible modal dialogue that we got that was finished I think in February then usability is the fourth one and I lost track of Dysale 5 usability is the own documentation all the code that goes into Drupal core needs to be documented to explain what it does that's what those are so we have this policy that issue thresholds no I think people are just ignoring me not raising their hands I just wanted to get an idea I was expecting a very different audience here I'm nervous about this conversation all day because I didn't have time to prepare for it so I apologize for those of you who are dozing in the back one thing we do with big changes we always have the requirement that all test cases pass what do you think about the short period of time we introduce a big change and tests may fail for a week for example I think that's extremely disruptive I mean like it basically forces all other core development around it and that's my gut reaction but at the same time if we can get automated testing in sandboxes I don't think that it's ever have we ever been in a situation where so tell me more about why you think that that might be a feasible idea and what problem you think that could solve because the reviewers could focus more on the actual implementation without seeing all these pieces in the patch that just fix something or change the syntax there and there so the patch gets huge and you could have a much smaller patch and then when there is consensus on that just put it in and then try to get really community involvement to fix all the other moving pieces and to get to a passing test state again I guess it's dangerous and it's also very hard to get someone to fix something that's already in like we found with the blocks patch like there's so many bugs with it still get in line so I don't know something that might work like that is if you had multiple patches in the issue so you had one that was these are the big things and then you had the other one like this is everything that's not quite as critical so they would be committed together like the issue wouldn't pass unless they were both together but you could review them separately so we people do that a couple times but I actually don't find it that useful personally maybe this is a personal thing for me what explains how something works is it's test coverage and if it's just an incidental test failure because it's using an old API that's where the BC layer comes in handy and we should fix that anyways to explain how our code works and it's not so much that I'm perfectly capable of like chopping the file without myself into different pieces to review I think that what's important is not just the file size it's what's included in it but I'm saying that it doesn't actually like I'm saying that the reason that it doesn't make a difference to me personally and this could be anyone else might have a different opinion about this but for me automated tests are part of explain the code does to me so that's my thought so for committing stuff that's not passing I think it's very dangerous because then you will not be able to commit anything else until it tests best again because you never know even if it's the same test same set of tests failing as before you never know if they are failing for different reasons or they are even worse stuff broken so I don't decide that would block core development on anything else until tests are fixed I mean if test bot fails randomly because there was an environment problem on a test bot when cores tested like core development just stops and everything that everyone's working on that day gets re-cued and you have to wait for hours for stuff to happen and we harass Alex and beg him to fix the change that was made that's now causing failures that kind of thing happens so yeah I'm I'm not convinced that there's any case in which that getting rid of the testing gate to the point that tests are actually failing would ever be a good idea personally so I think that so you could use sandboxes or other environments where you can get people to review your stuff without it passing that's fine and I think that you know you can even do something where you add a BC layer temporarily in your sandbox that you're going you tend to get rid of even in the same patch just so you don't have the noise of that test failures if you want people to review architecture early but like I mentioned for me having the documentation and testing those aren't an afterthought that's part of that if you want your code to do something there should be a test that explains how you do that so any questions comments so something I just wanted to point out is that in case you didn't know so right now Drupal core is Drupal 8 core is in feature freeze the code freeze is in five weeks so what that means is that we can't add major new features anymore to Drupal 8 we're done with that we have most of the feature set that we're probably going to have it released but we can still make API changes for contributed modules however something you might not have known is that it's actually possible still to add minor features to Drupal core if they if at this point they can still break APIs even just so long as they're not significant things that would introduce more architectural debt but in order to do that we have to actually fix other issues first we have to fix the major and critical issues until we get them down to an acceptable level so when I'm talking about all these big changes and so forth in some ways this isn't I mean at this point in the release cycle it's mostly just cleanup work we're doing we're probably going to see more of these 200 and 300 k patches as work in some like I'm sure that some of the work that's going on in to convert fields the new API is going to be involved significant changes and so forth you guys are really quiet no thoughts I'm looking over here no okay I just like spam a bunch of information at you and I thought you'd argue more I don't bite so if you've got a really big patch reach out to me when it gets started because it really helps if you talk to me yeah actually being available online this is something I skipped over because I decided I was blathering too much but being available online to talk about your patches is really valuable both for review ease and reviewers unfortunately that's not always possible what does online need oh ha so how many people in here use IRC okay that's better online in IRC in the Drupal-contribute channel that's where both core and contribute development kind of happens in Drupal it's sort of a hub of our communication and we use that as a work around for the fact that it's hard to communicate through core issues or any issues I guess I just wanted to confirm that one of your lessons was like how about guys that are working on a patch actually works I mean the part where watch it was like going crazy was the fact that you posted a review then we started commenting and then suddenly there was another review and it was impossible to follow it at that point and when you were in the sandbox making those little changes it was really really helpful so thank you for that I mean it took a long time and I don't I'm so I was trying to think about what strategies would make that easier because obviously there aren't that many people are even going to run into this problem because there aren't that many people who are willing to review a 200k patch but you know it's like so if I don't document for myself on the issue the things that I noticed I will forget by the end of the patch so it's like do I have to like in order to have a nicely formatted review do I have to then paste that in a handbook page on Drupal.org to save it until I have the opportunity to write that 3,000 word never again actually summary of everything like would that have been I mean it is like is it just having the comments there or wasn't that clear that I wasn't actually just done reading the whole patch yet yeah I think that was it I mean there were too many comments I mean we know that the issue queue is not that flexible and stuff like that we know that but I mean there were too many comments and there were a lot of comments but there was just too many and what's actually also sometimes annoying is that you get an email and I remember at some point reading my emails was like what the fuck and then I went to the issue queue and you added the comment and was like okay this is okay again so watch out for that people who subscribe to issues by email I don't understand I can't I wouldn't I don't even I can't imagine how you could possibly manage that much email but obviously people do it so filtering based on what I mean I don't know I don't get it that's okay we have any idea about how we manage these huge issues that have 200 comments 300 comments do you think it's a good idea to just force close them I think we did that a couple of times we did that with CMI yeah we're just an Drupal.org and the administrator steps in and just closes the issue is that a good idea well so I I mean we also do things where we like actually deleted all the noisy comments like deleted system message failing tests and people changing issue statuses and we managed to keep the blocks as plugins patch issue at one page for that reason but it was still 300 comments and issue summaries are also another work run for that noise factor so the problem then is that you have to move to the other issue you just need someone to summarize the entire status of that previous issue but it is a lot it's a lot less intimidating to go to like this one issue that has all this work done previously and say oh I can I can help with this I understand I don't have to read 300 comments to figure out what's going on it does require someone though to invest the effort of actually communicating what was done previously and posting whatever final patch was and I am hopeful maybe if you want we can talk about the issue Q improvements I am hopeful that in the future the Drupal.org issue Q will do a better job of exposing what's important on an issue as opposed to the noise but it's definitely a constant challenge for everyone because currently if an issue has 200 comments you can consider it that most of the time there is no way to get an actual committed patch out of it you just have to close it let's start again in a new issue basically pointing to the other one there is the whole discussion you can read there but we have now some better understanding let's start again yeah it's definitely something we've done and I don't know if we should do it earlier I don't know if that's feasible and so like one at least one of the issues in that situation I don't know I don't know if it would have been beneficial alright if no one else has any comments questions we have 10 minutes left okay so doing big even not as big as those but doing a review where you have many comments so that you don't crush people who worked on it but you might need to do it in parts is it at all helpful to say I made it one third of the way through I'm not going to come back I always say that in every single post I've read this part of the patch so obviously it didn't help them but it didn't help because they still have to take the time to look and see what I said even if they just read that part that you're commenting on and miss that one little line so do you prefer to get it all at once or you just would prefer to not get feedback wait here but if you have no comments at all there's performance problems and architectural problems and other problems that persist I don't think I have an answer to that because I haven't been in an issue where all comments from the review have been in more comment so I don't know I'm not sure whether it would have been a big help if everything was in one comment we would be so crushed because it would have been extremely long because it's 10 pages long I have to scroll for like hours to review it and what I did for the Blox's plugins issue is actually I told Chris online that I'm not done reviewing this so I posted these 10 comments over the course of two weeks and in each case I remembered list and I grouped them by like whether they were a coding standards issue at the bottom of the post or whether they were like a severe architectural question this is really broken looking at the top of the post and then after that like it said it took me a flight from London to Minneapolis to summarize what I had gone through in that patch so I'm sorry okay so I don't want to be blasphemic because the comments are not the place for code reviews you put the code is the place where the code review should occur so if you look at what a lot of other people use like we could talk about but there's a lot of other solutions like FishEye or Crucible or Garrett the review should be in the code and then it doesn't matter if there's one comment so like adding a to-do I actually I did do that in the field patch for things that I couldn't fix myself I the problem is that I also then the blocks patch and a lot of those to-do's are still there but yeah I actually I think that actually is helpful because then the person who's reading the issue doesn't have to go back and like go to that file go to that line and say first of all does she even know what the heck she's talking about secondly okay now I know what this changes to make and now I have to look and go back and look in context so in that case they can just apply the updated comment on patch yay they're not dead back there oh okay at the other on the other hand though that makes it harder for other people to see what you've already said so if someone had done that to one of my issues that would have been pissed at the amount of time everyone would have wasted because if you're just going through and changing my like unless you have are working in my sandbox which I always have a sandbox like I'm gonna make other changes and your inner diff is now useless to me and it would just waste my time to have to use that and not just have in the issue I mean I don't think comments are necessarily the place for it but if we had like github style comments I know I'm not supposed to talk about github you can now I'm done with github because it seems to be the answer to visually separate discussion and real review with like this is wrong you know if I miss a leading slash it's not up for discussion like that's different but I mean please don't do that to my issues don't fix, don't add to dos and then post a 180k patch with the dos of it I agree that's not what I was trying to suggest I was trying to suggest that if you seriously go and look at github go and look at fisheye, go and look at garret there are amazing code review solutions out there this problem has been solved hey Greg we have six minutes left anyone else have anything else mostly me just running through a bunch of stuff that I didn't really finish preparing for because I have too much of stuff to do but yeah, there's a lot of information so apparently I was expecting more people also for anyone who's not familiar the people who keep asking questions here this is gobor hoshi, he's the lead for the multilingual initiative the guy that just walked in is Greg Dunlap he's the lead for the configuration management initiative he's not here as a core maintainer Kathy down here is my favorite person in the world I have lots of favorite people but Kathy is a top contributor to the multilingual initiative and also a very active core mentor and let's see who else has questions Kristoff is that Swentell he is a co-maintainer now for the field api so these are all and Tim is one of the people who is in core team and also the top contributor to Drupal A right now so these are all what? Saint Tim I'm supposed to say Saint Tim now so it's obviously a very in-group discussion about these changes that we confront every day as people who are really active and just talking about it something that in the other half of my life that's not about this which is about trying to help people on the outside on this inside that talk about this in this secret language is trying to break down some of those walls and I feel like our issue queue has kind of become a wall because we're talking about all these strategies for how to work around things that it doesn't do well to help ourselves when we spend all day using this tool and how could anyone who doesn't do that every day possibly understand it I'm going to do more question asking so has anyone in here ever attempted to review a Drupal core patch at all okay that's a lot that's better okay good I was starting to kind of worried so that's good does anyone want to give me examples of issues that you've looked at and worked on you can shout it out don't worry don't bother with the mic because okay you know anyone? a lot of you raised your hand what's an example of an issue you worked on not these guys I know what they did aww surprise you're terrible feedback I'll share one documentation they're pretty easy to go in and sometimes there's a lot of politics that argue about this group no but sometimes there's less politics there than there is in the politics of the code what Jared was saying just to briefly summarize is that reviewing documentation patches for example changes and that even that can involve political discussion even for those fairly self-contained well-defined issues hey actually thank you I'm Dylan probably better known as Genzi just something I've worked on this winter was the the file some file metadata things and the file EPI and that's been one of the I don't know the answer but it's been a hard part for me as a contributor with less time probably than most of you just keeping up with like a big change so we were working on like we did a really terrible thing in Drupal 7 where we put the file metadata in the file reference thing so if you change that and you've got 10 different entities referencing the same file you've got to change it in 10 different places which so we were working on that and we kind of had a few weeks into it and had whiteboarded it and almost together what we thought was going to work and then all of a sudden it was like you know what we don't actually need that because there's going to be this new file some other file or some other metadata API so I don't know what the answer is it's just that like if you're you know maybe it's even just knowing that when someone's going in knowing like what's the scope of what can you take on personally and knowing like when you're ready to even get involved with huge you know mega patches or something like that and running into the situation where the metadata API has that landed like is it did actually get in because that's something that Gabor mentioned you know it's like you end up waiting for something that's new and shiny to solve your problem I don't know actually if it did land but that's yeah my name is John Antoine known as JAntoine on Drupal.org formerly known as Antoine Solutions I haven't reviewed a core patch but I tried to write a form element with Drupal 7 and ended up running into checks in code looking specifically for if this is a checkbox do this or if this is a radio do this and I was trying to work with checkbox and radios so it wasn't working so I didn't handle it correctly but it brought me back to the first session I attended which I've been pretty much in this room but it was Mark on unit testing and just hearing all of the core discussions unit tests and having OOP code I think would tremendously help the complexity that a lot of fresh Drupal lures deal with I hope so so I would say that right now in Drupal we have added a lot of OO in Drupal 8 I would say right now we've actually increased the complexity because we're in a situation where we have a mix of both where there's procedural either actual procedural code in classes and sometimes or at least procedural philosophically code in it and the community is still really learning how to do that because Drupal as a project comes from a time when PHP didn't really have good object support I mean like you couldn't really do object oriented code in PHP 4 and earlier in an effective way so we're just now I think Larry Garfield likes to say reporting reporting Drupal from being a PHP 4 application to being a PHP 5.3 application and we're still I think as a community learning what exactly that's going to mean for us so I hope you'll see more of that in Drupal 8 and I hope that it's not more confusing anyone else hey Eve I'm done that's it we're out of time which is good let's all go have dinner so why should I here built me a statue and she's a little bit smaller than might have been advertised and she has a whip here so if you get one of my reviews just remember be nice to me I worked really hard and I care about you and I care about your code and I want to help you get in I'm very sorry if I make your life for us that's all I'm done