New maintainer mandclu added and permissions updated
Welcome aboard
Actually the README exists on .txt format: https://git.drupalcode.org/project/memory_profiler/-/blob/2.0.x/README.t...
We can just modify to a markdown format if needed
Thank you so much for your contribution
Marking as needs review
Thank you so much @zeshan.ziya
Do you think we should make it work directly with v5, or continue with 4.x?
v5 is better
We'll focus updates on 5.x so makes sense use this version
Thanks everyone
Thanks for your contribution
Patch #2 is using Drupal:service()
but this class has construct so it can use dependency injection
It feels like you are making code suggestions just to get a commit credit
@jrockowitz you're wrong
- D.O don't have "commit credit"
- Currently is "issue credit"
- So doesn't matter who is the commiter
- And personally speaking I definitely don't need this
Please slow down with your reviews and patches
Sorry, it's a democracy
- It's open-source project
- Everyone are free to make contributions
- If you think that isn't useful, you as maintainer can remove the credits on field "Credit & committing" @see Granting credit to issue contributors →
- But you can't block any user to contribute if he/she wants
- You should respect everyone
- I recommend you to read Drupal Code of Conduct →
- If you don't agree with CoF or project license, feel free to go out and create a private project where you can block people and do everything you want
Aware that #60 needs works according to comment above
On this meantime, for any reason if someone needs to use on 8.x-4.7 tag, this is the patch
P.S. Making as hidden since tests needs to be done development branch
Aware that #60 needs works according to comment above
On this meantime, for any reason if someone needs to use on 8.x-4.7 tag, this is the patch
P.S. Making as hidden since tests needs to be done development branch
Merged
Keeping open to receive new updates from bot if necessary
Merged into dev branch
P.S. I'm going to keep issue open as RTBC to receive new automatic patches from Project Update Bot
Hello!
README
This project allows you to identify the browser and the device used. A class will be added to the HTML tag.
Source: https://git.drupalcode.org/project/browser_detect
Project page
This project allows you to identify the browser and the device used.
A class is inserted in the HTML tag so you can use as you need.
Source: https://www.drupal.org/project/browser_detect →
Screenshot of example: https://www.drupal.org/files/project-images/img_4.png →
Is actually useful
- Part of PHP CodeSniffer - Command Line Usage →
- Patch was created a long time ago
- On that time the goal was to learn/ practice workflow on community
- Also contribute with small fixes
- Definitely don't impacts the functionality
- We can say that the Priority is Minor
- But valid since is a framework that follows standards
- Anyway, as a maintainer you may not accept it, and it doesn't matter
- Just like everyone can be ignorant
- Last but not least; useless is someone answer an issue from 7 years ago :)
Definitely a good catch! Thanks for reporting that @justcaldwell
Probably is a conflict in the bootstrap library
Explanation
- When you put Modal to appear on
/admin/*
it loads BS automatically on admin pages - If your Modal isn’t /admin/* it don’t load
- When don’t load it works
- So probably is a conflict
If you want to make sure:
- Open Modal Global Settings
- Mark the settings to “don’t load bootstrap automatically”
- Save
- Put Modal in /admin/*
- Verify the gear icon
P.S. it isn’t the final fix, is just a test to confirm that is a conflict on BS library
If you already has BS library being loaded in other project your Modal can works even disabling this option
Strange. Just tested on simplytest.me and it worked as screenshot →
Do you mind confirming that is the same icon, please?
@uv516 question #1 wasn't answered
1) Modal works without "module_filter"?
for now isn't critical:
- This case seems a conflict with other project
- Isn't impacting everyone
- 300+ sites using 7.x-1.3 without issues
1) Not "Modal filter", but "Module filter" (look at the pictures)
Was a typo, but you understood the question and didn’t answer that
Seems a conflict
Questions:
- 1) Modal works without "Modal filter"?
- 2) Do you already have bootstrap loaded before Modal Page installed? If yes, which version?
Thanks
commited into 2.0.x. Ty
patch re-rolled
Needs reroll after 📌 Add the memory peak to the response header Needs review
Merged to the dev branch.
Thanks for your contribution
Adding in the roadmap for the next relese
committed into 2.0.x dev branch
Thanks everyone
Seems good. Adding into 🌱 Plan for Memory profiler 2.0.0 Active
Thanks everyone
If an existing maintainer has not commented, move the issue to the Drupal.org Project Ownership queue, by editing the “Project” field on this issue
Moving to Project Ownership due to lack of response
Thanks for your contribution @mahtab_alam
Marking as Needs Works because MR status is:
Merge blocked: fast-forward merge is not possible. To merge this request, first rebase locally
Bootstrap is great if your custom theme is already using it, but adding it to the front end globally causes a number of conflicts on my end
agreed, using Drupal Dialog API without dependency of BS would be amazing
If I have some time I'll see what I can contribute
Awesome! Really appreciated @jeffc518
Merged into the dev branch. Thanks a lot @mpaulo
Reviewed and seems good for me
I'm interested being a maintainer
Comment here explaining why you would like to maintain the module
Reasons
- Last commit +7 years ago
- Last stable release → for D7 only released +9 years ago
- +100 sites using according to usage statistics for Memory profiler →
Goals
- Release the new version with 📌 Drupal 10 compatibility Needs review
- Keep project updated and safe
confirm that you have security coverage opt-in permission
✓ Can opt projects into security advisory coverage ✅
Contributions
@edsoncarlos, @murilohp and I are working in an important project using memory_profiler.
List of contributions, activities and clean-up in the issue queue:
https://www.drupal.org/project/memory_profiler/issues/3366573
📌
Drupal 10 compatibility
Needs review
https://www.drupal.org/project/memory_profiler/issues/3018392
📌
Dependency namespacing in .info.yml file
RTBC
https://www.drupal.org/project/memory_profiler/issues/3366605
📌
Add the memory peak to the response header
Needs review
https://www.drupal.org/project/memory_profiler/issues/3228272
📌
Add core_version_requirement: ^8 || ^9 for Drupal 9
Postponed
https://www.drupal.org/project/memory_profiler/issues/3141070
📌
Automated Drupal Rector fixes
Closed: duplicate
https://www.drupal.org/project/memory_profiler/issues/3327996
💬
Offering to maintain Memory profiler
Closed: duplicate
#2 makes sense, +1 to go
@larruda huge pleasure talking to you, master! 👑
any reason for being config entities?
To use configuration management
Explanation
- Version 8.x-2.19 → is using Content Entity ( ref )
- Migrated to Config Entity on 4.x at #3142963: [PLAN] Change Modal Entity from Content Entity to Config Entity →
- Motivation was to export Modals with
drush cex
like webform - Was evaluated pros / cons of Content Entity x Config Entity
- At the time we didn't find a "hard-reason" to use Content Entity
- On the other hand, exporting feature was something interesting
For this reasons the migration but I'm open to consider using Content Entity if we have a good justification
About making Modal fieldable; I truly agree that would be an important milestone for the project's growth
thanks everyone
I’m happy to know that
So, closing the issue
After the upgrade have tried to run update.php and clear the caches?
Explanation
- The last version of 1.x is 1.3
- The last version of 2.x is 2.0.11
So this upgrade from 1.3 to 2.0.11 is the most common, and according to the project usage → , there are 21,616 using 2.x version, so it's curious that for them is working fine
I'd suggest:
- Upgrade from 1.3 to 2.0.11
- Run
drush updb
- Run
drush cr
If didn't work, my recommendation as workaround:
- Upgrade from 1.3 to 2.0.0
- Run
drush updb
- Run
drush cr
- Upgrade from 2.0.0 to 2.0.11
Merged. Thanks everyone
Thanks @gautam_105
But I think the MR wasn't created correctly, right?
Please take a look
Aware about the explanation on #37
On this meantime, if someone needs a workaround for D10.x, this patch may help you
Actually I think it wasn’t fixed since the patch wasn’t committed, right?
Moving back the previous status
Woow, thank you so much for this amazing contribution @jnlar
On my first look in the MR, seems good
I'm moving to "Needs Review" just to manual tests
The maximization feature is op-in and disabled by default, in ModalForm.php
Pre-existing/new modals will need to check 'Enable Maximize Button' and then save the edit modal form in order to have the button display in their modal.
That's perfect!
With that solution it won't impact the existent sites but the feature will be available and we'll let them know on release notes
I think the enable_modal_header dependency is sensible as it follows the same pattern as other buttons (close button) placed in the header region.
Agreed!
Since this button is located on Modal Header I think makes sense to include there
Thanks for the patch @bburg
Moving to Needs Review to test that and commit into 5.x branch
Appreciate
Awesome job @jnlar, thank you so much
I'm marking as "Needs Works" because if we merge that today, will appear this new button in all modals of 2k+ sites
So before merging that we need 2 thinks:
- Create an option inside of Modal CMS for Enable Modal Maximization (Default "true")
- Create a hook_update_N(): Load all existent Modals and set Enable Modal Maximization as
false
With that it won't impact the existent sites, but if they want to use they'll be able to enable that on Modal entity via CMS. Is that makes sense?
P.S. if prefer we can create a separated issues for this
Committed the info files, so seems that on Drupal the project is ready but now we need to update the "Janrain REST" to be compatible: https://github.com/janrain-rest/janrain-rest
Any contributions will be welcome
Backing to NW since we need to update the info file
Committed to the dev branch and created a new release 4.1.0-beta1 → available for D10
Thanks
Seems good
Moving to 4.1.x-dev, with the plan to launch 4.1 version with D10 support
Awesome! Thanks @tguerineau , we’ll test it and provide feedbacks
I think we can implement the same improvement on this other method to be consistent
renatog → created an issue.
Moved to the dev branch. Thank you so much for your great job @tguerineau
Adjusted phpcs
It's working fine, just come points on phpcs
FILE: modal_page/src/Controller/ModalAjaxController.php
-------------------------------------------------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
-------------------------------------------------------------------------------------------------------------------------------------
101 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements
104 | ERROR | [ ] The array declaration extends to column 85 (the limit is 80). The array content should be split up over multiple
| | lines
112 | ERROR | [ ] The array declaration extends to column 84 (the limit is 80). The array content should be split up over multiple
| | lines
118 | ERROR | [ ] The array declaration extends to column 108 (the limit is 80). The array content should be split up over multiple
| | lines
130 | ERROR | [x] Expected newline after closing brace
136 | ERROR | [ ] The array declaration extends to column 115 (the limit is 80). The array content should be split up over multiple
| | lines
-------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------------------------------
Time: 959ms; Memory: 14MB
Moved to the dev branch
@kevinquillen thanks for your feedback. It's a pleasure having your presence with great suggestions in the issue queue. I hope to see more
Best
better name in the variable eventKey
Moved to the dev branch
@kevinquillen thanks for your feedback. It's a pleasure having your presence with great suggestions in the issue queue. I hope to see more
Best
Patch with the fix suggested on IS
Thanks for the patch @tguerineau, let's review the code and test it
Agreed! Thanks
Until this ticket is implemented as a workaround we can use config ignore → to disconsider the auto-complete configs
Thanks for your feedback @kevinquillen
No cookies are ever written when a user clicks "OK" or in this case re-labelled to "I Accept"
The project wasn't designed to create cookies on "OK" button, so isn't a "bug", but I definitely is a great suggestion so I'm converting to Feature Request
Workaround
For now as a temporary you can use HOOK_modal_ID_alter()
and when the user clicks on "I accept" we can store the cookie programmatically
Example:
/**
* Implements hook_modal_ID_alter().
*/
function HOOK_modal_ID_alter(&$modal, $modal_id) {
// Set cookie to don't show again
$cookieName = 'please_do_not_show_again_modal_id_' + $modal_id';
// TODO: logic to insert the cookie.
}
Other possibility:
- Edit this Modal(s)
- Put a class on "OK button" E.g.
i-accept-modal
- Hide the checkbox
please don't show again
for the end-users withdisplay:none
- Create a custom JS to mark this checkbox when the user clicks on "I Accept" class
I also see no way to control the cookie name for each modal
We also don't have this option, I created a ticket to implement that ✨ Create a way to customize the cookie name for "don't show Modal" Active
Question: If we have one config inside "Global Settings" to customize the cookie is enough, or do you think is necessary to customize cookie name from Modal to Modal?
Is there any reason why the keydown event is not as simple as
I think there is no reason. Good catch btw, thanks for reporting @kevinquillen
Thanks for providing more information
Let’s try to reproduce
Moved to the dev branch
@tguerineau great suggestion of using JSON instead of "integer" on AJAX response. I created this separated issue 📌 Update the ModalAjaxController::hookModalSubmit() to return JSON instead of int value Fixed and we can work there
Thanks for reporting and providing feedbacks, we really appreciate
first problem: undefined method Drupal\Core\Extension\ModuleHandler::getImplementations()
is the same of
#3293221: D10: ModuleHandler::getImplementations() deprecation →
second one: Input value contains a non-scalar value
is the same of
🐛
Ajax pager - Input value "viewsreference" contains a non-scalar value
Fixed
This patch contains the same fixes
Are there any pre-requisites for using modal module?
No, there isn't
I installed it on my production site also but there also the page admin/structure/modal does not open
Strange, I just installed and this page is working for me - ( screenshot → )
P.S. I also tested changing the configs on "Global Settings" but continue working
Do you mind pasting the log of the error, please?
1) Go to /admin/modules
and enable the "Database Logging"
2) Open the page "
/admin/structure/modal
" to see the "blank page"
3) Open the page: "/admin/reports/dblog
"
4) Get the log of error and paste here, please
It'll helps in the investigation
Committed to the dev branch
Thanks for your contribution @gawalin - good catch!
Tested the patch and it really works well
That's nice! Thanks for the patch, let's test it
I tested and worked for me
@qsurti as #2 could you please provide a "step by step" to reproduce, please?
@robbt , this one happens only on 5.0.3?
I'm wondering if this is side-effect of the new release
I saw that the description was in the "tags" field. So just moving to the description
P.S. If someone else can validate if is happening for you as well, please?
Make 2.x default branch on Gitlab
Marking as duplicate in favour of 📌 Set default branch to 2.0.x Active
Thanks everyone
Thanks everyone
The new release https://www.drupal.org/project/page_manager_search/releases/2.0.0-beta5 was created at 🌱 Plan to 2.0.0-beta5 Fixed and it's available with this fix
Release available at https://www.drupal.org/project/page_manager_search/releases/2.0.0-beta5 →