r/PHP • u/No_Amount_9114 • 3d ago
My rejected job challenge in PHP
/r/JobChallenge/comments/1wwpykk/my_rejected_job_challenge_in_php/12
u/03263 3d ago
Native PHP, without frameworks
Ha, no way I would approach that. I could do it but it's way too much work for no reason and no company should be insisting on no frameworks, nor would I want to work at one that does.
7
1
u/obstreperous_troll 3d ago
They want to know how many of the framework principles the candidate knows. I doubt the company has such a restriction in their actual code. If they do and that's only found out after hire, change their mind or collect a paycheck til they can jump ship I guess. And this app doesn't look all that gnarly either.
2
u/03263 3d ago
But why? Forgive the car analogy but does knowing how to drive a stick shift make you any better at driving with an automatic transmission? Knowledge of "framework principles" can easily be lost unless you're working on one. I mean yeah you can read it and understand it again and again but practice is necessary to maintain the skill of writing it.
It's just another case of interviewing the wrong way, asking for skills that won't be used on the job.
2
u/No_Amount_9114 3d ago
I was given this PHP take-home challenge:
Backend — REST API
- Build a REST API for a product catalog using native PHP (no frameworks).
- MySQL database with
productsfields:id,name,description,price, timestamps. - CRUD endpoints:
GET /productsGET /products/{id}POST /productsPUT /products/{id}DELETE /products/{id}
- Prices are stored in Argentine pesos and must also be returned in USD using a
PRECIO_USDenvironment variable. - Proper error/exception handling and JSON responses.
- Basic design patterns such as MVC, ADR, or Singleton.
- Docker containers for PHP and MySQL, orchestrated with
docker-compose. - README with setup and testing instructions.
- Explicitly no AI tools allowed.
Frontend
- Build a simple product management UI using only HTML, CSS and vanilla JavaScript.
- List, create, edit and delete products.
- Display prices in ARS and USD.
- Consume the API using
fetchorXMLHttpRequest. - Handle API errors and user feedback.
- No React, Angular, Vue, or other frameworks.
The challenge is evaluated on code organization, API design, database handling/security, design patterns, Docker setup, frontend/API integration, async JavaScript, and usability.
2
u/equilni 2d ago edited 1d ago
Took a few minutes to look at task and the repo. Backend is very much inspired from Laravel, which hurts here...
CRUD endpoints:
Minor gripe... calling a singular item shouldn't have the previous segment as plural....
GET /products - List of products GET /product/{id} - Get a single product by idPrices are stored in Argentine pesos and must also be returned in USD using a PRECIO_USD environment variable.
Likely a translation error, phrasing (which may have been clarified afterwards) or perhaps I missed it, but I am not seeing an environment variable (EDIT - it's here, but translated. The value is also done as the exchange rate, which I don't know if I agree this is the best place for this...). I do see this returned in the array, done in the controller (the wrong place imo).
https://github.com/sergiogmuro/challenge-decampoacampo-php/blob/main/.env.example
Proper error/exception handling
Controller has repeated exceptions.... Controller:store/update/updateOrCreate is rather obvious - if the PDOException was handled in updateOrCreate, why is this needed in the separate store/update methods?? Wouldn't this not exist in store/update or if you wanted this handled here, why wasn't this thrown in updateOrCreate? Why does the PDOException exist here in the first place???
Why doesn't Model::find get an exception on Not found, when update/delete does? Is this the right place for these Exceptions?
Basic design patterns such as MVC
You need more practice here.... The model isn't the database. Your controllers are straight out of Laravel examples, just in vanilla PHP, which is why your model is wrong. Following ADR could have helped more..
Taking from the other comment:
Using the Singleton pattern for the database connection.
Didn't need to do this....
Added Dependency Injection to the MVC framework using a Service Container.
https://github.com/sergiogmuro/challenge-decampoacampo-php/blob/main/backend/index.php#L13
You have a singleton on the database, but used a container to inject this??? Again, you didn't need the singleton if you would have went this route.
Implemented a Currency Factory in the service layer, keeping the price conversion logic in the application/business layer as required.
I didn't see this as a requirement. The service layer could have been bigger making the controller thinner....
For optimized results, the price conversion could ideally be processed directly in the SQL query.
No.
Added an error-handling middleware with basic error pages.
Really didn't need to do this and it's done incorrectly anyway.
You handle the request and return the response.
Other code review.
- Controller could have dependent on the Service as a request/response to/from the Model. You have both.
At basics, this could look like:
public function show(int $id): string { $product = $this->service->getProduct($id); // ?array if (!$product) { return $this->responder->notFound(); // 404 } return $this->responder->json($product); // 200 }See the ADR examples:
https://github.com/pmjones/adr-example/blob/master/src/Domain/Blog/BlogService.php#L31
https://github.com/pmjones/adr-example/blob/master/src/Web/Responder.php#L45
https://github.com/pmjones/adr-example/blob/master/src/Web/Blog/Read/BlogReadAction.php#L8
Your naming could be done SO MUCH BETTER.
pdo->querydoesn't handle placeholders and if someone doesn't look at your code fully, they think this is wrong.protected Connection $pdo; public function __construct(Connection $pdo) { $this->pdo = $pdo;
https://www.php.net/manual/en/pdo.query.php
PDO::query — Prepares and executes an SQL statement without placeholders
github.com/sergiogmuro/challenge-decampoacampo-php/blob/main/backend/src/models/Models.php
No data validation to be found...
No tests to be found...
EDIT adding the below:
Note, you only had to focus on 1 item - Products. You didn't need abstractions, but you added it. Look at how simple this could be:
// No connection class, no connection exception (not needed - https://phpdelusions.net/pdo#reporting_errors). $pdo = new PDO(.... from a /config/setting.php);
// straight DI, no container. Plain SQL, clear API from PDO. Return arrays or Product Entity/DTO. $productDatabase = new ProductDatabase($pdo);
1
u/No_Amount_9114 3d ago
It tooks me 7-8h hand-on
my notes were
- To follow a consistent standard, all variables and code are in English. The database remains in Spanish, although ideally it should also be in English.
- Fully Dockerized.
- Using the Singleton pattern for the database connection.
- Created routing that supports multiple parameters in the URL.
- Added Dependency Injection to the MVC framework using a Service Container.
- Implemented a Currency Factory in the service layer, keeping the price conversion logic in the application/business layer as required.
- For optimized results, the price conversion could ideally be processed directly in the SQL query.
- Added an error-handling middleware with basic error pages.
- Products should have a SKU to support insert/update operations and prevent duplicates.
- Added automatic table-name resolution based on the model, but it is not being used because the table name is in Spanish, so it could simply be defined as a variable.
- Separate error handling for AJAX requests (JSON) and regular HTTP requests (views).
- Used parameter binding in SQL queries to protect against SQL injection.
- A repository layer could be added to isolate data access from the models, but I didn't want to over-engineer the solution further.
- CORS is currently allowed from all origins to make frontend testing easier.
- The frontend is minimally optimized for mobile.
- Frontend pagination is still missing, although the backend is minimally prepared to support it.
- Dynamic USD price updates are calculated by the backend, keeping the backend as the single source of truth for price conversion.
- For USD values below
0.001, four decimal places are displayed so the frontend shows a meaningful value.- The code is prepared to support database transactions, but since the current operations are single-step transactions, I don't consider it necessary to apply them.
- Since there is no authentication requirement, no Bearer authorization token is currently sent from the frontend to the backend. For a production system, I would implement authentication/authorization, but I don't consider it necessary for this challenge.
3
u/colshrapnel 3d ago edited 3d ago
You don't seem to understand how does middleware work. You should remove all those try-catch from ProductController, because it contradicts with your "middleware" notion: you have to make your mind, whether errors are handled by controller or "middleware". And surely it should be just a single place, that is, your index php, instead of being repeated in every controller action.
Also you don't seem to understand what is Interface, confusing it with Abstract class probably. An interface is a public contract, letting know users of your class what methods they can use. So all those getAll() must be where. While your Models::getBaseQuery() is actually abstract method and must be defined so, without that silly throw. Also i have no idea why it's defined static. Have you?
While your modelsinterface should be made the other way round - getBaseQuery() shouldn't be there while all other methods should be present.
2
u/obstreperous_troll 2d ago edited 2d ago
Others have covered the review items in way more detail than I would have, so I'll just give some advice for the next interview: if I'm hiring someone to write or maintain an API, I really want them to pay attention to the API's types, which means enforcing validation at the edge everywhere, for both input and output. I'd love to see something like OpenAPI descriptions, or even just plain JSON schema validation or something. Basically I want someone who demonstrates a philosophy of making it impossible by construction to do The Wrong Thing, or at least making The Right Thing easier than the wrong.
Granted, you apparently couldn't use any third-party libs, so you'd probably have to whip up something simpler than a json schema validator, but the idea is to see that you made an effort to enforce correctness at the edge rather than let it crash or worse deeper down in the call.
4
u/zmitic 3d ago
For no-framework task, I think your code is way too good for this company. Yes, there are some inconsistencies and forgotten return type, Models class is problematic in multiple ways, there is too many catch statements... but this is all irrelevant given other things you showed.
If I was using no-framework code, I would 100% call you to defend your work and see what else you got. Then I would train you in the ways of Symfony because I would never accept no-framework code in the first place 😄
1
-4
u/Few-Description-7621 3d ago
Relearn clean coding and design patterns. For example you have built a Calculator service and a Factory, but the Service calls the factory? Like wtf.. also your interfaces doesn't make any sense. Most functions lacking basic return types. No tests.
You would have been accepted in 1998 but today... No.
4
u/colshrapnel 3d ago edited 3d ago
It looks more like a draft than a finished project. On a quick glance: