Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PanneauPocketBridge] Use dynamic list of cities #3845

Conversation

floviolleau
Copy link
Contributor

Hi,

I think I spotted an issue on the AbstractBridge

I added some comment why I was forced to copy/paste a core method or maybe I misunderstood the logic.

Feel free to add comments

Copy link

github-actions bot commented Dec 18, 2023

Pull request artifacts

Bridge Context Status
PanneauPocket 1 untitled (current) ✔️
PanneauPocket 1 untitled (pr) ⚠️ WARNING Undefined array key 0 at lib/BridgeAbstract.php line 175
⚠️ WARNING foreach() argument must be of type arrayobject, null given at lib/BridgeAbstract.php line 175
⚠️ WARNING Attempt to read property "href" on null at bridges/PanneauPocketBridge.php line 32
⚠️ WARNING Attempt to read property "plaintext" on null at bridges/PanneauPocketBridge.php line 37
⚠️ DEBUG lib/FeedItem.php(118): Expected $uri to be string but got NULL
⚠️ (shutdown) 8192: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in bridges/PanneauPocketBridge.php line 38

last change: Monday 2023-12-18 19:53:14

@EVOTk
Copy link

EVOTk commented Dec 18, 2023

I've just tried it, and it works well for me! Thanks

@dvikan
Copy link
Contributor

dvikan commented Dec 18, 2023

lint

@dvikan
Copy link
Contributor

dvikan commented Dec 19, 2023

thanks for the contribution flovi.

this pr needs improvement.

  1. this pr increases the frontpage size from 1.7M to 2.3M (25%) because of the 13000 options in the list. this not good

  2. the bridge does http call in constructor. not good

  3. use cache to cache the big list of cities

  4. i agree there is a bug in parent method regarding static::PARAMETERS. if you feel brave can you please fix it so we dont have to copy paste the method.

  5. i dont really see a need to do work in the constructor

@floviolleau
Copy link
Contributor Author

floviolleau commented Dec 26, 2023

Hi,

For item 1, what do you suggest?
For item 2, I don't see other solution to initialize the const cities.
For item 4, here is the PR #3858

Thanks for the review

@dvikan
Copy link
Contributor

dvikan commented Mar 31, 2024

sorry dont have mental energy to followup this one

@dvikan
Copy link
Contributor

dvikan commented Jun 18, 2024

  1. rss-bridge might need a new feature to support this. a feature which uses ajax to dynamically autocmplete a long list.

we simply cannot render thousands of <option> elements in the html directly.

  1. we could possibly add this PR behind a bridge setting (config.ini.php) which must be opt-in

feel free to make a new PR

@dvikan dvikan closed this Jun 18, 2024
@floviolleau
Copy link
Contributor Author

floviolleau commented Dec 8, 2024

Hi,

I made a PR to accept a new type for parameters: dynamic_list (@see #4361).

I created a new PR accordingly. Merge #4362 only if #4361 is merged.

Kind regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants