@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.
We are going to need a CR for this, i think.
@ignaciofarre It's not ready for review since there is still a failing stage in the pipeline. See #12. Have you tried fixing it?
#71 Read with interest. Thank you.
Re 35, checked 'summary lines' made edits. I am ok with RTBTC.
Edited MR, PHPSTAN errors x2
Edited MR, addressed last code comment.
Pipeline running green except for test-only test which fails as it should.
Another idea might be to keep as we generally do, a close eye on the Symfony project. How are they implementing HTMX? And with what velocity? Slow-burn, or swallowing it hook line and sinker.
#68 In my analogy there is no one maintainer. I assume it is an open source library like Drupal? So it is various committees who make the decisions. No one person?
Even if that were to exist evaluating new libraries is still valuable because they may meet the needs of a project better and iterate faster.
Absolutely. Your comments are valid. I am rejecting nothing. I am raising objections: questions that should be answered. If the vehicle analogy is not applicable then I would like to know why not.
Sometimes there is a better way of doing things and adopting a library can help standardize and developers not familiar with drupal can pick it up quicker.
Sometimes, indeed. I think we should have a clear roadmap of how the adoption of such a library or more like the removal of the AJAX library will benefit the project in the ways you describe. Can you give me any concrete examples?
#66 And the alternative is to keep using AJAX which will itself gradually absorb from HTMX. We just leave AJAX in place and leave AJAX to make all the cherry-picking decisions and do the heavy-lifting for us. Then any HTMX features will have passed through AJAX projects gates. If we gradually transition to HTMX then that would (in a sense) violate DRY or DRWTAD! : ) (Dont repeat what they are doing.)
#64 HTMX is heralded as the next AJAX? If so then in the Darwinian world of software development there are two possible outcomes.
- HTMX ascends and AJAX descends
- Before long HTMX replaces AJAX
The other possible outcome is:
- AJAX integrates HTMX, it cherry picks the best bits.
There are more nuanced outcomes eg in the case of Javascript libraries many have been 'all the rage' for a year or two then disappeared into obscurity. But in this case it seems like HTMX has only one competitor: AJAX.
The analogy with investment is useful because a project like Drupal.org has an infinite amount of time its largely volunteer workforce can devote to Drupal core development. That time is a scarce resource. Maybe we should wait for HTMX to achieve what I assume it intends to: to replace AJAX as the defacto standard for what AJAX does. In the long-run that will save many hours devoted to swapping out AJAX with HTMX.
Edited the IS.
@nicxvan Ah, I see. Thank you for fixing.
Rebased and resolved merge conflict.
Hi @1cg Just noticed #60. I assume you are liaising with other CMS open source projects? Any projects you can list that are taking HTMX on board?
Dealt with 2 of 3 comments in the MR. Rebased.
Is there not a danger that somewhere down the road of implementing HTMX it gets integrated into AJAX library or a subset of it? In the same way that AI searches are becoming integrated into search engines and becoming easier by the day to integrate into other software libraries/ components.
@quiteone RE: #30, updated the CR. Is the new version closer?
Needs test coverage.
Updated the IS but more extensive edits are required.
Edited MR. Changed 'up-branch' to 'parent'. However perhaps it should be 'hierarchy of parent terms' or 'complete parent hierarchy'?
Updated the IS. Have left '[]' placeholders where further information is needed.
Edited the issue summary. But there are several fields still blank.