- Issue created by @dydave
- Status changed to Needs review
8 months ago 2:52pm 18 July 2024 - 🇫🇷France dydave
This is my initial stab at this:
I've created an initial merge request so I could get a patch to apply on my project, but I definitely wanted to run this by you Conrad (@cmlara).
I didn't have much time to fix the problem, investigate why/when/how this change occurred in related core libraries and understand what is really happening under the hood or the global implications/impact these changes could have on the module in general, so your review, advice and feedback would be super appreciated.
So far, the patch seems to fix the problem for image styles with my setup: Private bucket all the way and everything is fed through Streamwrapper (public and private files).
The error has disappeared and the images are displayed properly.
I'm aware, the merge request would definitely need more work, in particular, updating comments, variable names, etc...
But I wanted to know if these changes were going in the right direction first, before spending more time on this...I'm setting this in Needs review so you could perhaps let me know at a first glance if there is anything I've changed that could cause other potential issues.
Based on your advice, feedback, suggestions and recommendations, I would definitely be very happy to help testing, or making any change to the merge request, if that could help you in any way.
Otherwise, feel free to let me know if you have any questions or would need more information on the bug itself, the ticket in general, or the merge request, I would surely try answering as soon as possible.
Thanks very much in advance! - Status changed to Needs work
8 months ago 5:18am 19 July 2024 - 🇺🇸United States cmlara
Overall looks like you have the right idea on implantation changes required.
✨ Add support for D10.3 Active has some of the equivalent changes from the 8.x-3.x branch (classes are in different locations but have the same code heritage).
Glancing through your changes it looks like you may have caught a few areas in the code that I missed (phpdocs) when updating the 8.x-3.x branch as I don’t recall updating those. We probably actually do need old and new references for the time being in the docs as both are allowed and may be processed through.
I’ll note that the S3fsFileSystemD103 is the new class for 10.3+. It is mostly the same as the pre10.3 class and probably very close to the MR with some added BC of mapping INT’a to ENUM’s that probably should exist in our service too so that way we are natively speaking in cores target typedef with no need to worry about core dropping INT’s in D12.
As we are not in alpha/beta we can make changes to the file system service without a new class so that is fine as is.
I’ve been considering next week after core releases 11.x we could (since we have not even released an alpha yet) bump our minimum requirements to 11.x and avoid some of the BC logic that 3.x needed to have related to
Long term it helps us to have a 4.0.0 release with as little BC layers as possible. Once we release I tend to hold that we do not drop support for a version of core until we next increase our major (right now 8.x-3.x can work on D8.8-D10.3 and with a small set of commits sitting in the NR queue will also support D11 as context of what we are working with regarding commitment to BC)
Setting as needs work as I belive it will need at least the enum/int BC mapping changes brought in (we should convert inside our service as eventually core will drop integers and we will need to be sure to always pass ENUM’s to core) and a decision on what to bump core minimum too will determine if any other layers are needed.
- 🇫🇷France dydave
Thanks a lot Conrad (@cmlara), once again for the prompt feedback and for taking the time outline the necessary changes and roadmap, it's super appreciated! 🙏
This one is going to take me a bit more time, to review all the points, related issue and make another round of changes to the MR 😅
At least I know my initial round isn't going to break my site and is a first step in the right direction 👍
I'll be following-up on this issue soon!
Thanks!