Added description to @return value (see #99). The return value was in fact described at the top of the docblock (instead of a general description of what the function does). Rejigged text of the docblock trying to avoid repetition and to satisfy PHPCS
Fix bot line length warnings
Fixed bot warnings, 80 chars comment line length; rebased.
@berdir Or assertEquals()? assertEquals
There is an example at 1.9 of usage applied to arrays.
We are now using MR's instead of patches. The MR's are made against the 11.x branch and can then be back-ported to other versions.
oily → changed the visibility of the branch 3506427-remove-responsiveimage.ajax-from-10-4 to hidden.
Re: #63 There are two tests in the user module.
Re: #66 The thread has been resolved. Test coverage seems also ready for review. Back to 'Needs review.'
@mdranove I see the test-only test is now failing and only one other test is failing. Worth re-running. For #63 hopefully the test code that seems to work can be moved/ refactored into the user module.
@mdranove Just ran the test-only test (you have to run it manually). It is passing but it should fail. What that normally means is the test coverage is passing before and after the code fixes in the MR are applied. That is not happening. Changing to 'needs work'.
@mdranove You may have had a particular reason? but normally it's best to stick to one branch per issue. Anyway checking your latest changes.
I added thread to the usercancelTest.php. There is a function that makes PHPSTAN fail as it doesnt exist.
I understand from #6 that any non-test files with a @return void should have the @return removed. i have committed that one edit. The other @return void blocks are in test files so i have left those alone.
Corrected one typo then remembered the etiquette so added two threads: code comment nits.
Ran the 'test-only' test. It fails as it should. Pipeline otherwise all green.
@smustgrave I think I may have jumped in. I saw #34 'done' so checked and saw one failing test. Wasnt sure it was a random failure but since just experienced a single random failure in another issue, re-ran and it passed. So ran 'test-only' test. It passes. Since there is test coverage in the MR, that should not happen? So 'needs work' to make the test effective?
Perhaps I should have commented to @finnsky to carry out these steps? But from experience explaining the test-only test, why to run it, how to run it etc. seems so much harder than just running it and then explaining what the result means..
Rebased 11.x MR
Created an 11.x branch. Copied the the 10.x MR changes to the new MR.
@tdroden Looks good. We need to fork from the 11.x-dev branch. Any changes to 10.x will be backported from 11.x-dev. I am not certain that this will need a PHPUNIT test. I am removing the tags and changing to 'Needs review'. The reviewer will hopefully decide if PHPUnit test coverage is needed.
@smustgrave Sorry I just saw the timestamp on #55. I was sure it was more than 48 hours ago. Just rebased. Fixed conflict that was in only one line of one of the 2x telemetry test files in umami profile FunctionalJavascript tests. I changed the actual value from 7 to 4 because 4 was the expected value. With the value 7 the test failed with the value 4 it now passes.
Not certain that this was the correct action to take from a technical perspective. Should not have done it anyway as should have allowed 48 hours to elapse.
The comments seem clearer now in line with recommendations.
@the_g_bomb I hate that when you go all around the houses. Life sucks sometimes : )
@berdir Ah, interesting it's even more complicated than i thought. Perhaps including the link would help.
it only removes the ability to invalidate a whole bin
That seems a really important point at least for my personal understanding.
deleteAll() in redis has exactly the same problem as invalidateAll(), it also doesn't actually delete anything and just sets a timestamp
Okay but this issue does not make any difference to that Redis problem? It is a Redis problem that is much harder to deal with? Even though the nature of the problem is very similar.
I agree with #6 that the CR is quite readable. But perhaps like Hayden would perfectly understand Mozart's explanation of a passage, the CR is a IMO a little hard to grasp for an average musician.
I have edited it and tried to put it in terms that I understand which has the advantage of showing what I am not grasping. Using the supermarket shelf analogy, I am unsure about what happens when the item is deleted (incinerated). It seems that with Redis etc a label is placed on the shelf stating 'this item has been incinerated'? Not sure about Drupal standard caching, though.
#8 I notice that core.entity_view_display.node.article.default.yml version in Nightwatch profile contains core.entity_view_display.node.article.default.yml
But it is not contained in the same file in standard profile. Havent looked at the umami version of the file yet.
#8 Yes I was confused. I thought it was a configuration profile for Nightwatch itself. But I suppose umami could be useful at some point. The article edit url seems fine especially since there is an existing url for the 'Page' node type that uses the same query param.
Wondering if trying without the query param (I read something about the query param being required to POST the form?) which i think would just load the form? To try to isolate the cause: is it the query param in the url? I think the issue is currently isolated to the article edit url being added to the test. If we can isolate it to the query param that would be slight progress?
@the_g_bomb Grepping the core codebase I notice there are not apparently any existing usages of the 'standard' Nightwatch profile showing up. What is the 'standard' profile? Is that a configuration that has been standardised for Drupal? Or is it the default configuration profile that Nighwatch itself provides whether Nighwatch is being used on Drupal, Wordpress etc?
I have conducted a detailed analysis of existing deprecation notices of methods in the codebase.
There is a pattern evident in the content of every existing notice. They contain two separate types of information.
- A statement that the method is removed.
- Further information
The further information states one and one only of the following::
- There is no replacement
- The method or technique or enum that should now be used instead
The MR I encountered contained 'a statement that the method is removed.' It did not contain 'further information'. So I added the 'further information'. This 'further information' I have drawn attention to in my new comments. I am not sure it is correct.
@usingsession By all means! Remember the #testing channel on Slack, too. You should get good support.
Added comments to the MR.
RE: #27
@oily I was just seeking confirmation and discussing it with you. No worries at all—
Oh, good. So if i write a comment directed as @smustgrave stating that your MR should be removed and a new one created and you keep creating MR's that need to be replace, you would be cool with that?
The fact is that someone is going to need to find out why this method is being deprecated. And decide whether it is because it is 'Not needed anymore.' or 'the method has no replacement' because every single deprecation notice in the core codebase of this kind contains such a comment. Or else, they need to state why all the other deprecation notices are including unnecessary detail.
RE: #22
@smustgrave Can you please review the last commits? It seems these changes might not be necessary, as this method is not used anywhere.
In that case, I suggest that this notice I have just found in the core codebase contains the message that we need in your MR. (I grepped and found 2x notices stating 'Not needed anymore..')
* @deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. Not
* needed any more.
I suggest that you update the MR and replace my additional text with 'Not needed any more.'
RE: #25
@oily I was referring to this ticket: #3497796.
As I stated in #23 issue 3497796.is not related to this issue as a parent, child or in any way at all.
If you want to start a fight about 'you did this on issue arandomticket, and you did that on issue anotherrandomissue I am not interested!
But since you seem to want to knock this back to Needs to Review or is is Needs Work go ahead!
@shahini_jha Perhaps you should look at what others are saying about me:
I also did some minor edits (mostly just formatting) on the CR, and agree that looks really good.
@shalini_jha RE #22
I noticed that @oily has made similar changes in another ticket, which also seem unnecessary.
We are dealing with this issue unless that one is related? What I do in other tickets is immaterial. I work extremely hard on behalf of drupal.org on a great many different tickets. You fail to mention that at #19 I have updated the IS.
Search existing deprecation notices in the core codebase and you'll find it very normal to state the method being replaced or that none is being replaced.
@catch Ah, okay. Thanks.
Added comments to the MR.
@vidorado After welcome visits from @longwave and @catch in the same day we'll probably have to wait a month now : ).
@larowlan That definitely looks promising. However have worked quite a lot on the related issue, I believe @tame4tex's is more likely to fix it too. If this is closed I have no problem with opening a feature request so long as the code fix she created can be accessed. Danger with closed works as intended is people abandon it not knowing it contains a working solution. I do believe I tested her fix and it worked. Postponed might be better.
Good work! But i think NR is correct.
Pipeline is green.
Added in deprecation notice that the method has no replacement. I assume it does not. But if it does have a replacement we need to state what it is.
@usingsession I just re-ran the failing test. I recommend re-running if there are one or two random failures.
@catch #45 I'm not sure this involves 'stale CSRF links'? Is it not about the limitations on how routes can be structured? Certain types of routes that contain placeholders used to allow CSRF generation but now they will no longer support CSRF generation? Something like that..
#45
Ok I'm out. I asked to stop putting words in my mouth and you instantly turn around and do it again.
I have never put any words in your mouth. You are making baseless allegations. That in itself is annoying.
Your comments are a great example on how to achieve that.
That is an utterly baseless allegation. This together with the abo ve is a targetted attempt to humiliate me on the platform and to make me appear to be a wrongdoer.
I suggest you consult your solicitor. If you are considering continuing this bullying.
@ignaciofarre #16 No need. Great work so far.
@kristiaanvandeneynde No, please stay! : )
@kristiaanvandeneynde RE: #43 First of all, I did not supply the code to fix this. @tame4tex did. I have tried to fix the related issue. I used a code approach (to begin with) supplied by @catch, a one-liner. It did not fix it. Others tried varations on the one-liner. None work.
I do not fully understand the fix you are proposing. The way it works in Open source in general is if we think we can fix it we create the code and run it through the pipeline. Until your fix is through the pipeline and it runs green etc how does anyone know it works? I wish it was all that easy!
#39 There's a saying 'the customer's always right'. #37 details user complaints.
This behavior has led to confusion among users and, in our experience, unnecessary client support time spent addressing their concerns
There are many issues lacking any specific customer interraction feedback like #37. I think that should elevate the importance of this issue and the 2x related ones. I dont think the site setup in #37 is unique. Probability is there are other sites like it with customers feeling the same way.
In that case there are two related issues. Based on your reasoning they should be closed too. But if we do that then any bug if minor should be closed too.
https://www.drupal.org/project/drupal/issues/3255711 🐛 Log out show error message "... your browser must accept cookies ..." Needs work
In that case there are two related issues. Based on your reasoning they should be closed too. But if we do that then any bug if minor should be closed too.
#41 If they are bothered im sure they will let us know : ).
It is nitty but grammatically
This allows checking for legacy CSRF tokens and treat them as valid, too.
should be
This allows checking for legacy CSRF tokens and treating them as valid, too.
@vidorado I am reading through the comment in the last commit. I see what you mean with the 'it'. I think it is quite a complex message you are putting across. I must ahve read it 20 times and tried to understand it in relation to your comments before i understood. Not sure why but i was initially thinking 'method' meant 'a way of doing things' rather than 'a function'. But at least by throwing in the 'it' you were able to see that I was misunderstanding the comment and to identify what it was that i was misunderstanding.
Pipeline green, test only test fails as it should.
@dcam just re-ran the one failing test
Thanks, I will check that. Just seen a youtube where you explain the concepts.
Hi @1dcg, RE: #77 second paragraph. Can you please enlarge on what looks like the most immediate way you envision that Drupal can benefit from GHC? Im patenting that new? abbreviation GHC by the way :). You can use it but only with my permission : ).
Edited MR. Attempt to make code more comprehensible.
oily → changed the visibility of the branch 10.3.x to hidden.
Ok, I see.
@rkoller Sounds good. We could change status of this to 'postponed'?
@rkoller Can you look at line 89 of https://git.drupalcode.org/project/drupal/-/merge_requests/11003/diffs
I mean it looks like you might need to edit line 89 of the gitlab.yml. Not sure you edited the right line?
All threads seem resolved. RTBTC?
Pipeline green. Test-only fails as should.
Edit MR. Resolve @vidorado's comment.
200+ commits behind 11.x. Time for rebase, methinks.
232 commits behind 11.x. Rebase.
No probs. I think the 'problem' you mention was in an issue that got fixed today? I have worked on a few issues today and re-run a few pipelines not had any probs.
Pipeline all green.
@rcodina #151 So why did you not re-run the one failing? I done it. I note the test-only test is failing. Great!
Edited the CR, reworded in places: to emphasise the new possibilities the change provides!
Thank you @joachim. Great work on this issue!
Not sure if you guys use the pipeline 'test-only' test. You have to run it manually. It is a smart test. It will only run any test code in the MR, ie it does not run the 'fix' code. That way you can see if you have effective test coverage. Effective in the sense that the test will fail if applied to 11.x branch before this MR is merged. But pass when the MR is merged into it.
I reviewed the MR. I noted that the new test assertion will pass before and after the removal of the description in the MR. I have added an assertion that asserts that the page does not contain the text of the description. That should fail without this MR and pass with it.
Reviewed the code. No comments added. Test-only test fails as it should. All green
Can we please provide more info on the DRY violation. How many methods are repeated and roughly what % of the code is repeated?
@dqd I prefer 'light sparring' no heat at all : )
@rkoller Have you linked this in one of the Slack channels? You can reach out to Javascript aficionados to get one more review before RTBTC.
#40 I just viewed the last pipeline. The test-only test is failing as it should. This is close to RTBTC, now. You seem to have resolved all threads.